Bug 230801

Summary:

[JSC] Optimize PutByVal with for-in

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

New Bugs

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

230815

Bug Blocks:

Attachments:

Description	Flags
Patch	saam: 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>