Summary: | Implementing caching transition puts that need to reallocate with indexing storage | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | barraclough, commit-queue, ggaren, keith_miller, kling, mark.lam, mhahnenberg, msaboff, oliver, rniwa, saam, sam | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 156352 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Filip Pizlo
2014-03-28 15:58:13 PDT
Seeing many put_by_id caching failures on Speedometer due to this. OK, this is happening. Created attachment 275912 [details]
work in progress
Of course it doesn't even compile
Created attachment 275921 [details]
starting to work
Created attachment 275931 [details]
the patch
Attachment 275931 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:179: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:186: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:989: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1144: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1157: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 5 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 275931 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=275931&action=review r=me with comments > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:85 > + m_liveRegistersForCall.merge(extra); I think this should come before the above exclude because if the extra is part of a callee save, we shouldn't save it. And it probably shouldn't have any registers in common with RegisterSet::stackRegisters() or RegisterSet::reservedHardwareRegisters() > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1108 > + extraRegistersToPreserve.set(baseGPR); > + extraRegistersToPreserve.set(valueRegs); These will be locked registers in the scratch register allocator. I wonder if its a bug that ScratchRegisterAllocator doesn't consider locked registers as part of its used register set. Maybe it should? What are your thoughts? > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1147 > + jit.emitExceptionCheck(CCallHelpers::InvertedExceptionCheck); Style: This can be one line. Not something to do in this patch, but I wish we had better names for exception checks. InvetedExceptionCheck means nothing to me. I'd like "JumpIfException" and "JumpIfNotException" or something similar. (In reply to comment #7) > Comment on attachment 275931 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275931&action=review > > r=me with comments > > > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:85 > > + m_liveRegistersForCall.merge(extra); > > I think this should come before the above exclude because if the extra is > part of a callee save, we shouldn't save it. > And it probably shouldn't have any registers in common with > RegisterSet::stackRegisters() or RegisterSet::reservedHardwareRegisters() You're right, I fixed it. > > > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1108 > > + extraRegistersToPreserve.set(baseGPR); > > + extraRegistersToPreserve.set(valueRegs); > > These will be locked registers in the scratch register allocator. > I wonder if its a bug that ScratchRegisterAllocator doesn't consider > locked registers as part of its used register set. Maybe it should? > What are your thoughts? I don't think it's a bug. The other user of the call preservation stuff will have consumed its inputs before it makes its call. So, those inputs (just baseGPR for custom getter, or baseGPR and valueRegs for custom setter) should only be saved if they are live *after* the call. This is quite different, since the call is happening early. We haven't used the baseGPR and valueRegs yet. We will still use them after the call. I think that the better default is the one we have now, because it's more customizable. Otherwise, the custom getter/setter stuff would have to say that baseGPR and valueRegs are excluded. Except that this would have a strange meaning because we would be saying that they are excluded from the set that was added when we added locked registers. For that to work right, the custom getter/setter code would have to go to great lengths to list *all* of the registers that were locked, including scratchGPR. It would actually have to know exactly what the locking discipline was in the outermost scratch register allocator. I think that would be weirder than what I do in this patch. > > > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1147 > > + jit.emitExceptionCheck(CCallHelpers::InvertedExceptionCheck); > > Style: This can be one line. > > Not something to do in this patch, but I wish we had better names for > exception checks. InvetedExceptionCheck means nothing to me. > I'd like "JumpIfException" and "JumpIfNotException" or something similar. That would make sense. The "inverted" thing made sense at the time because almost all exception checks fall through when there is no exception. What are the perf number from this? (In reply to comment #9) > What are the perf number from this? Up to 5x faster on microbenchmark. Still running the larger benchmarks. This is likely to only be profitable for in-browser tests, and I haven't run any of those yet. This is neutral on small JS benchmarks like Octane and such. Comment on attachment 275931 [details] the patch Clearing flags on attachment: 275931 Committed r199209: <http://trac.webkit.org/changeset/199209> All reviewed patches have been landed. Closing bug. |