Bug 153792

Summary: [JSC] Introduce LinkTimeConstant mechanism
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, ews-watchlist, gyuyoung.kim, joepeck, keith_miller, mark.lam, msaboff, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 203795    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch saam: review+

Description Yusuke Suzuki 2016-02-02 11:16:36 PST
...
Comment 1 Yusuke Suzuki 2019-11-02 21:39:05 PDT
Created attachment 382690 [details]
Patch
Comment 2 Yusuke Suzuki 2019-11-02 21:47:42 PDT
Created attachment 382691 [details]
Patch
Comment 3 Yusuke Suzuki 2019-11-02 21:56:02 PDT
Created attachment 382692 [details]
Patch
Comment 4 Yusuke Suzuki 2019-11-03 00:18:52 PDT
Created attachment 382695 [details]
Patch
Comment 5 Yusuke Suzuki 2019-11-03 00:30:21 PDT
Created attachment 382696 [details]
Patch
Comment 6 Yusuke Suzuki 2019-11-03 00:58:17 PDT
Created attachment 382699 [details]
Patch
Comment 7 Yusuke Suzuki 2019-11-03 01:00:45 PST
Created attachment 382700 [details]
Patch
Comment 8 Yusuke Suzuki 2019-11-03 01:11:17 PST
Created attachment 382704 [details]
Patch
Comment 9 Yusuke Suzuki 2019-11-03 01:49:27 PST
Created attachment 382705 [details]
Patch
Comment 10 Yusuke Suzuki 2019-11-03 01:58:41 PST
Created attachment 382703 [details]
Patch
Comment 11 Yusuke Suzuki 2019-11-03 02:14:34 PST
Created attachment 382706 [details]
Patch
Comment 12 Yusuke Suzuki 2019-11-03 17:20:14 PST
Created attachment 382711 [details]
Patch
Comment 13 Yusuke Suzuki 2019-11-03 18:47:50 PST
Created attachment 382713 [details]
Patch
Comment 14 Saam Barati 2019-11-04 12:20:20 PST
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?
Comment 15 Yusuke Suzuki 2019-11-04 18:47:54 PST
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.
Comment 16 Yusuke Suzuki 2019-11-04 18:52:08 PST
Committed r252032: <https://trac.webkit.org/changeset/252032>
Comment 17 Radar WebKit Bug Importer 2019-11-04 18:53:20 PST
<rdar://problem/56890056>
Comment 18 Yusuke Suzuki 2019-11-04 19:30:31 PST
I guess Windows build failure is due to broken forwarding header thing
Comment 19 Yusuke Suzuki 2019-11-04 19:32:23 PST
(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.