Bug 130914

Summary: Implementing caching transition puts that need to reallocate with indexing storage
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: 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 Flags
work in progress
none
starting to work
none
the patch none

Description Filip Pizlo 2014-03-28 15:58:13 PDT
...
Comment 1 Andreas Kling 2015-04-28 14:32:17 PDT
Seeing many put_by_id caching failures on Speedometer due to this.
Comment 2 Filip Pizlo 2016-04-07 11:39:33 PDT
OK, this is happening.
Comment 3 Filip Pizlo 2016-04-07 12:13:45 PDT
Created attachment 275912 [details]
work in progress

Of course it doesn't even compile
Comment 4 Filip Pizlo 2016-04-07 13:09:13 PDT
Created attachment 275921 [details]
starting to work
Comment 5 Filip Pizlo 2016-04-07 14:09:24 PDT
Created attachment 275931 [details]
the patch
Comment 6 WebKit Commit Bot 2016-04-07 14:11:39 PDT
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 7 Saam Barati 2016-04-07 15:10:07 PDT
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.
Comment 8 Filip Pizlo 2016-04-07 15:15:58 PDT
(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.
Comment 9 Oliver Hunt 2016-04-07 15:17:40 PDT
What are the perf number from this?
Comment 10 Filip Pizlo 2016-04-07 15:18:41 PDT
(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 11 WebKit Commit Bot 2016-04-07 19:11:50 PDT
Comment on attachment 275931 [details]
the patch

Clearing flags on attachment: 275931

Committed r199209: <http://trac.webkit.org/changeset/199209>
Comment 12 WebKit Commit Bot 2016-04-07 19:11:55 PDT
All reviewed patches have been landed.  Closing bug.