Bug 130914 – Implementing caching transition puts that need to reallocate with indexing storage

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

[Filip Pizlo]

...

[Andreas Kling]

Seeing many put_by_id caching failures on Speedometer due to this.

[Filip Pizlo]

OK, this is happening.

[Filip Pizlo]

Created attachment 275912 [details]
work in progress

Of course it doesn't even compile

[Filip Pizlo]

Created attachment 275921 [details]
starting to work

[Filip Pizlo]

Created attachment 275931 [details]
the patch

[WebKit Commit Bot]

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.

[Saam Barati]

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.

[Filip Pizlo]

(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.

[Oliver Hunt]

What are the perf number from this?

[Filip Pizlo]

(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.

[WebKit Commit Bot]

Comment on attachment 275931 [details]
the patch

Clearing flags on attachment: 275931

Committed r199209: <http://trac.webkit.org/changeset/199209>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.