| Summary: | GetByVal should use polymorphic access and hook into a status object | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 203693 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 203865, 203866, 204082, 204392 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Description
Saam Barati
2019-10-09 15:06:22 PDT
Created attachment 382886 [details]
WIP
Created attachment 382970 [details]
WIP
Created attachment 382983 [details]
WIP
it cached some things
Created attachment 383005 [details]
WIP
Created attachment 383011 [details]
WIP
Created attachment 383103 [details]
WIP
Works for strings and array loads.
Need to do:
- arguments object loads
- typed array loads
- string[index] loads
Created attachment 383147 [details]
WIP
Created attachment 383163 [details]
WIP
scoped/direct arguments loads work now
Created attachment 383185 [details]
WIP
Implemented direct/scoped argumetns
Implemented typed arrays
Going to do strings next
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
Created attachment 383286 [details]
WIP
Now handles symbol properties
Created attachment 383300 [details]
WIP
piping it through DFG/FTL
Created attachment 383305 [details]
WIP
I think it now works in all the tiers.
Created attachment 383329 [details]
WIP
Fixed a bug where we did the wrong thing by loading a union member and used the wrong |this| :-(
Created attachment 383332 [details]
WIP
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
Created attachment 383402 [details]
WIP
GetByIdStatus is starting to work
Created attachment 383416 [details]
WIP
some renaming
Created attachment 383524 [details]
WIP
It's almost done. Just looking into a bug that only happens with the concurrent JIT.
Created attachment 383529 [details]
WIP
Created attachment 383534 [details]
WIP
Created attachment 383535 [details]
WIP
Created attachment 383592 [details]
WIP
might be done. Just need to clean a few things up.
Created attachment 383594 [details]
WIP
test EWS
Created attachment 383595 [details]
WIP
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 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
It’s neutral on JetStream2. Testing Speedometer2 now Seems like it's a 1% speedo2 regression. Investigating... Created attachment 383664 [details]
WIP
I think I have fixed the speedometer2 regression Created attachment 383810 [details]
WIP
Created attachment 383819 [details]
patch
Created attachment 383821 [details]
patch
I have some tests and microbenchmarks I will upload in a bit, but this patch is ready to be reviewed rebasing now Created attachment 383822 [details]
patch
rebased
Created attachment 383824 [details]
patch
Created attachment 383826 [details]
patch
Created attachment 383831 [details]
patch
fix scoped arguments part after index masking being rolled out
Created attachment 383892 [details]
patch
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. Created attachment 383901 [details]
patch
remove bad debug assert I forgot to delete
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. 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. Created attachment 383936 [details]
patch for landing
Created attachment 383937 [details]
patch for landing
Comment on attachment 383937 [details] patch for landing Clearing flags on attachment: 383937 Committed r252684: <https://trac.webkit.org/changeset/252684> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 384073 [details]
Patch
(In reply to Caio Lima from comment #52) > Created attachment 384073 [details] > Patch can we use a different bug for this? Comment on attachment 384073 [details]
Patch
Oops, uploaded to the wrong bug...
|