[webkit-reviews] review granted: [Bug 153792] [JSC] Introduce LinkTimeConstant mechanism : [Attachment 382713] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 153792: [JSC] Introduce LinkTimeConstant mechanism
https://bugs.webkit.org/show_bug.cgi?id=153792

Attachment 382713: Patch

https://bugs.webkit.org/attachment.cgi?id=382713&action=review




--- Comment #14 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 382713
  --> https://bugs.webkit.org/attachment.cgi?id=382713
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382713&action=review

r=me with a few comments

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:903
> +	   case SourceCodeRepresentation::LinkTimeConstant:

maybe we should have global object in the name of this enum value since it's
dependent on global object?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:838
> +    , m_linkTimeConstantRegisters(numberOfLinkTimeConstants, nullptr)

This seems wasteful of memory. Any given compile should only have a few of
these at most on average. Why not just use a HashMap instead of this Vector?
(If we keep the vector, we might as well make it have this as the inline
capacity since it's statically known exactly how big the vector will be. But I
think HashMap is better.)

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:467
> +macro loadConstant(size, index, tag, payload)

can other code that branches on if it's a constant use this helper (below in
the LLInt)?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:469
> +macro loadConstant(size, index, value)

can the code below use this as a helper?

> Source/JavaScriptCore/runtime/JSGlobalObject.h:423
> +    Vector<LazyProperty<JSGlobalObject, JSCell>> m_linkTimeConstants;

why not have the correct inline capacity? Or do we really want this out of
line?

> Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h:113
> +inline JSFunction* JSGlobalObject::throwTypeErrorFunction() const { return
bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::throwTypeErrorFunc
tion)); }
> +inline JSFunction* JSGlobalObject::newPromiseCapabilityFunction() const {
return
bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::newPromiseCapabili
ty)); }
> +inline JSFunction* JSGlobalObject::resolvePromiseFunction() const { return
bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::resolvePromise));
}
> +inline JSFunction* JSGlobalObject::rejectPromiseFunction() const { return
bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::rejectPromise)); }
> +inline JSFunction* JSGlobalObject::promiseProtoThenFunction() const { return
bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::defaultPromiseThen
)); }
> +inline JSFunction* JSGlobalObject::regExpProtoExecFunction() const { return
bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::regExpBuiltinExec)
); }
> +inline GetterSetter* JSGlobalObject::regExpProtoGlobalGetter() const {
return
bitwise_cast<GetterSetter*>(linkTimeConstant(LinkTimeConstant::regExpProtoGloba
lGetter)); }
> +inline GetterSetter* JSGlobalObject::regExpProtoUnicodeGetter() const {
return
bitwise_cast<GetterSetter*>(linkTimeConstant(LinkTimeConstant::regExpProtoUnico
deGetter)); }

why not static cast for all of these?

>
LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-val
ue-expected.txt:17
> +  this: c - 

Why has code you have in this patch changed the backtrace?