Bug 202767 – GetByVal should use polymorphic access and hook into a status object

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

[Saam Barati]

...

[Saam Barati]

Created attachment 382886 [details]
WIP

[Saam Barati]

Created attachment 382970 [details]
WIP

[Saam Barati]

Created attachment 382983 [details]
WIP

it cached some things

[Saam Barati]

Created attachment 383005 [details]
WIP

[Saam Barati]

Created attachment 383011 [details]
WIP

[Saam Barati]

Created attachment 383103 [details]
WIP

Works for strings and array loads.

Need to do:
- arguments object loads
- typed array loads
- string[index] loads

[Saam Barati]

Created attachment 383147 [details]
WIP

[Saam Barati]

Created attachment 383163 [details]
WIP

scoped/direct arguments loads work now

[Saam Barati]

Created attachment 383185 [details]
WIP

Implemented direct/scoped argumetns
Implemented typed arrays

Going to do strings next

[Saam Barati]

Created attachment 383191 [details]
WIP

ok, I think all the ICs are implemented. What's left to do:
1. make DFG/FTL emit this inline cache
2. hook into the status objects

[Saam Barati]

Created attachment 383286 [details]
WIP

Now handles symbol properties

[Saam Barati]

Created attachment 383300 [details]
WIP

piping it through DFG/FTL

[Saam Barati]

Created attachment 383305 [details]
WIP

I think it now works in all the tiers.

[Saam Barati]

Created attachment 383329 [details]
WIP

Fixed a bug where we did the wrong thing by loading a union member and used the wrong |this| :-(

[Saam Barati]

Created attachment 383332 [details]
WIP

[Saam Barati]

Created attachment 383366 [details]
WIP

Ok, it seems to pass all tests. Now I need to hook it into a status object and then I think it could be done

[Saam Barati]

Created attachment 383402 [details]
WIP

GetByIdStatus is starting to work

[Saam Barati]

Created attachment 383416 [details]
WIP

some renaming

[Saam Barati]

Created attachment 383524 [details]
WIP

It's almost done. Just looking into a bug that only happens with the concurrent JIT.

[Saam Barati]

Created attachment 383529 [details]
WIP

[Saam Barati]

Created attachment 383534 [details]
WIP

[Saam Barati]

Created attachment 383535 [details]
WIP

[Saam Barati]

Created attachment 383592 [details]
WIP

might be done. Just need to clean a few things up.

[Saam Barati]

Created attachment 383594 [details]
WIP

test EWS

[Saam Barati]

Created attachment 383595 [details]
WIP

[EWS Watchlist]

Comment on attachment 383595 [details]
WIP

Attachment 383595 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13257609

New failing tests:
animations/no-style-recalc-during-accelerated-animation.html
fast/workers/worker-cloneport.html

[EWS Watchlist]

Created attachment 383604 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Saam Barati]

It’s neutral on JetStream2. Testing Speedometer2 now

[Saam Barati]

Seems like it's a 1% speedo2 regression. Investigating...

[Saam Barati]

Created attachment 383664 [details]
WIP

[Saam Barati]

I think I have fixed the speedometer2 regression

[Saam Barati]

Created attachment 383810 [details]
WIP

[Saam Barati]

Created attachment 383819 [details]
patch

[Saam Barati]

Created attachment 383821 [details]
patch

[Saam Barati]

I have some tests and microbenchmarks I will upload in a bit, but this patch is ready to be reviewed

[Saam Barati]

rebasing now

[Saam Barati]

Created attachment 383822 [details]
patch

rebased

[Saam Barati]

Created attachment 383824 [details]
patch

[Saam Barati]

Created attachment 383826 [details]
patch

[Saam Barati]

Created attachment 383831 [details]
patch

fix scoped arguments part after index masking being rolled out

[Saam Barati]

Created attachment 383892 [details]
patch

[Saam Barati]

Comment on attachment 383892 [details]
patch

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

> Source/JavaScriptCore/bytecode/GetByStatus.cpp:345
> -    for (ICStatusContext* context : icContextStack) {
> +    if (icContextStack.size()) {

this old code seems clearly wrong. We're asking about ICs from the top most frame in random inline call frames (including possible doing OOB access on the instruction stream!). I'll verify with Phil if perhaps I'm missing something and there are cases where this helps.

[Saam Barati]

Created attachment 383901 [details]
patch

remove bad debug assert I forgot to delete

[Keith Miller]

Comment on attachment 383892 [details]
patch

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

r=me with some comments.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:734
> +            jit.loadPtr(MacroAssembler::Address(propertyGPR, JSString::offsetOfValue()), scratchGPR);

How do we know the string is uniqued? Do we always unique strings? Or do we always hit the slow path?

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:464
> +    for (auto& accessCase : cases) {
> +        if (accessCase->needsScratchFPR()) {
> +            state.scratchFPR = allocator.allocateScratchFPR();
> +            break;
> +        }
> +    }

I doubt it matters but it seems like this could be a bit on the PolymorphicAccess itself.

[Saam Barati]

Comment on attachment 383892 [details]
patch

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

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:734
>> +            jit.loadPtr(MacroAssembler::Address(propertyGPR, JSString::offsetOfValue()), scratchGPR);
> 
> How do we know the string is uniqued? Do we always unique strings? Or do we always hit the slow path?

We know it's a unique string because it'll be pointer equal to the cached string of the access case. If it's not unique, we'll hit the slow path. (AccessCase stores a ref to a uniqued string implies)

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:464
>> +    }
> 
> I doubt it matters but it seems like this could be a bit on the PolymorphicAccess itself.

I don't think that's quite right, since this depends on the cases, which may change for the same polymorphic access instance.

[Saam Barati]

Created attachment 383936 [details]
patch for landing

[Saam Barati]

Created attachment 383937 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 383937 [details]
patch for landing

Clearing flags on attachment: 383937

Committed r252684: <https://trac.webkit.org/changeset/252684>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57348889>

[Caio Lima]

Reopening to attach new patch.

[Caio Lima]

Created attachment 384073 [details]
Patch

[Saam Barati]

(In reply to Caio Lima from comment #52)
> Created attachment 384073 [details]
> Patch

can we use a different bug for this?

[Caio Lima]

Comment on attachment 384073 [details]
Patch

Oops, uploaded to the wrong bug...