Bug 230801 - [JSC] Optimize PutByVal with for-in
Summary: [JSC] Optimize PutByVal with for-in
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 230815
Blocks:
  Show dependency tree / graph
 
Reported: 2021-09-26 01:08 PDT by Yusuke Suzuki
Modified: 2021-09-26 22:02 PDT (History)
8 users (show)

See Also:

Attachments
Patch (37.95 KB, patch)
2021-09-26 02:36 PDT, Yusuke Suzuki 		saam: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>