Bug 153792 – [JSC] Introduce LinkTimeConstant mechanism

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 382690 [details]
Patch

[Yusuke Suzuki]

Created attachment 382691 [details]
Patch

[Yusuke Suzuki]

Created attachment 382692 [details]
Patch

[Yusuke Suzuki]

Created attachment 382695 [details]
Patch

[Yusuke Suzuki]

Created attachment 382696 [details]
Patch

[Yusuke Suzuki]

Created attachment 382699 [details]
Patch

[Yusuke Suzuki]

Created attachment 382700 [details]
Patch

[Yusuke Suzuki]

Created attachment 382704 [details]
Patch

[Yusuke Suzuki]

Created attachment 382705 [details]
Patch

[Yusuke Suzuki]

Created attachment 382703 [details]
Patch

[Yusuke Suzuki]

Created attachment 382706 [details]
Patch

[Yusuke Suzuki]

Created attachment 382711 [details]
Patch

[Yusuke Suzuki]

Created attachment 382713 [details]
Patch

[Saam Barati]

Comment on attachment 382713 [details]
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::throwTypeErrorFunction)); }
> +inline JSFunction* JSGlobalObject::newPromiseCapabilityFunction() const { return bitwise_cast<JSFunction*>(linkTimeConstant(LinkTimeConstant::newPromiseCapability)); }
> +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::regExpProtoGlobalGetter)); }
> +inline GetterSetter* JSGlobalObject::regExpProtoUnicodeGetter() const { return bitwise_cast<GetterSetter*>(linkTimeConstant(LinkTimeConstant::regExpProtoUnicodeGetter)); }

why not static cast for all of these?

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

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

[Yusuke Suzuki]

Comment on attachment 382713 [details]
Patch

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

Thanks!

>> 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?

Talked with Saam. For now, I'll use LinkTimeConstant since it represents constant populated at linking time.

>> 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.)

Right. Changed this to HashMap.

>> 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)?

Fixed.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:469
>> +macro loadConstant(size, index, value)
> 
> can the code below use this as a helper?

Fixed.

>> 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?

We cannot get this information in JSGlobalObject.h since it is included and used before generating JSBuiltins.h. We should not use LinkTimeConstant's JSBuiltins.h information here.

>> Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h:113
>> +inline GetterSetter* JSGlobalObject::regExpProtoUnicodeGetter() const { return bitwise_cast<GetterSetter*>(linkTimeConstant(LinkTimeConstant::regExpProtoUnicodeGetter)); }
> 
> why not static cast for all of these?

I use jsCast for JSFunction while I continue using bitwise_cast for GetterSetter.
Using static_cast / jsCast requires complete type declaration of GetterSetter here, but GetterSetter.h is not exposed outside of JSC.

>> LayoutTests/inspector/debugger/tail-deleted-frames/tail-deleted-frames-this-value-expected.txt:17
>> +  this: c - 
> 
> Why has code you have in this patch changed the backtrace?

I think this part is not dumping meaningful value before this change. If you change "c" to "d" in this test, this value becomes "test". And "test" as thisValue seems also incorrect. My guess is that this is related to hash table's value.
But anyway, I'll file a bug and ask to inspector folks.

[Yusuke Suzuki]

Committed r252032: <https://trac.webkit.org/changeset/252032>

[Radar WebKit Bug Importer]

<rdar://problem/56890056>

[Yusuke Suzuki]

I guess Windows build failure is due to broken forwarding header thing

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #18)
> I guess Windows build failure is due to broken forwarding header thing

Yes, seems like it is.