Bug 230801

Summary: [JSC] Optimize PutByVal with for-in
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 230815    
Bug Blocks:    
Attachments:
Description Flags
Patch sbarati: review+, ews-feeder: commit-queue-

Description Yusuke Suzuki 2021-09-26 01:08:22 PDT
[JSC] Optimize PutByVal with for-in
Comment 1 Yusuke Suzuki 2021-09-26 02:36:17 PDT
Created attachment 439293 [details]
Patch
Comment 2 Saam Barati 2021-09-26 09:29:59 PDT
Comment on attachment 439293 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1503
> +    OpJeqPtr::emit(this, cond, moveLinkTimeConstant(nullptr, LinkTimeConstant::sentinelString), target.bind(this));

No need for this to be a link time constant. It can live on UnlinkedCodeBlock. It’s not related to global object.
Comment 3 Yusuke Suzuki 2021-09-26 21:13:29 PDT
Comment on attachment 439293 [details]
Patch

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

Thanks

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1503
>> +    OpJeqPtr::emit(this, cond, moveLinkTimeConstant(nullptr, LinkTimeConstant::sentinelString), target.bind(this));
> 
> No need for this to be a link time constant. It can live on UnlinkedCodeBlock. It’s not related to global object.

Changed.
Comment 4 Yusuke Suzuki 2021-09-26 21:14:32 PDT
Committed r283095 (242153@main): <https://commits.webkit.org/242153@main>
Comment 5 Radar WebKit Bug Importer 2021-09-26 21:15:22 PDT
<rdar://problem/83557863>
Comment 6 WebKit Commit Bot 2021-09-26 21:23:03 PDT
Re-opened since this is blocked by bug 230815
Comment 7 Yusuke Suzuki 2021-09-26 21:23:36 PDT
Comment on attachment 439293 [details]
Patch

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

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1503
>>> +    OpJeqPtr::emit(this, cond, moveLinkTimeConstant(nullptr, LinkTimeConstant::sentinelString), target.bind(this));
>> 
>> No need for this to be a link time constant. It can live on UnlinkedCodeBlock. It’s not related to global object.
> 
> Changed.

Ah, no. This should be link-time-constant. Otherwise, it does not work with bytecode cache. I'll revert the patch and reland the original one.
Comment 8 Saam Barati 2021-09-26 21:25:19 PDT
(In reply to Yusuke Suzuki from comment #7)
> Comment on attachment 439293 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439293&action=review
> 
> >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1503
> >>> +    OpJeqPtr::emit(this, cond, moveLinkTimeConstant(nullptr, LinkTimeConstant::sentinelString), target.bind(this));
> >> 
> >> No need for this to be a link time constant. It can live on UnlinkedCodeBlock. It’s not related to global object.
> > 
> > Changed.
> 
> Ah, no. This should be link-time-constant. Otherwise, it does not work with
> bytecode cache. I'll revert the patch and reland the original one.

I see... That's unfortunate. I guess you could also make it so that we test equivalence to the "sentinel string" when serializing bytecode, and then store it as some kind of marker. But, just making it a link time constant sounds simple enough.
Comment 9 Yusuke Suzuki 2021-09-26 22:02:52 PDT
Committed r283098 (242156@main): <https://commits.webkit.org/242156@main>