[webkit-reviews] review granted: [Bug 229229] [JSC] Polymorphic PutByVal : [Attachment 436429] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 229229: [JSC] Polymorphic PutByVal
https://bugs.webkit.org/show_bug.cgi?id=229229

Attachment 436429: Patch

https://bugs.webkit.org/attachment.cgi?id=436429&action=review




--- Comment #28 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 436429
  --> https://bugs.webkit.org/attachment.cgi?id=436429
Patch

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

Nice. r=me with a few comments.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1068
> +	   if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)

assert it's invalidGPR?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1170
> +	   if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)
> +	       allocator.lock(stubInfo.m_arrayProfileGPR);

assert it's invalidGPR?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1257
> +	   if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)
> +	       allocator.lock(stubInfo.m_arrayProfileGPR);

assert it's invalidGPR?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1492
> +		   failAndIgnore.append(jit.branchIfNaN(state.scratchFPR));

Why ignore? Shouldn't this be a case to repatch, since we're not storing int32?
Or maybe we expect this particular array to get transitioned into something
more general, so we can kind of ignore the likelihood of this?

I guess I'm just worried about the case where we see a lot of non numbers in
practice? Like, say we started off with profiling a single number case, but
then we ended up with many things that are never numbers, we'll never end up
repatching. Not sure what the right approach is, but it's worth considering if
we should do something else here.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1505
> +		   failAndIgnore.append(jit.branchIfNotInt32(valueRegs));

ditto regarding repatching vs not repatching.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1573
> +	       state.failAndIgnore.append(jit.branchIfNotInt32(valueRegs));

ditto about repatching or ignoring.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1582
> +	      
state.failAndIgnore.append(jit.branchIfNotNumber(valueRegs.payloadGPR()));

ditto

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1594
> +	   state.failAndRepatch.append(jit.branch32(CCallHelpers::AboveOrEqual,
propertyGPR, scratchGPR));

we fail and repatch here, but ignore the case for normal arrays when we go OOB
past vector length. Is that intentional?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:2434
> +	   if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)
> +	       allocator.lock(stubInfo.m_arrayProfileGPR);

Assert invalidGPR?

> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp:145
> +    if (stubInfo.m_arrayProfileGPR != InvalidGPRReg)
> +	   allocator.lock(stubInfo.m_arrayProfileGPR);

assert is invalidGPR?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6475
> +		       tryCompileAsPutByOffset = true;

let's rename this to "compiledAsPutPrivateNameById"

> Source/JavaScriptCore/jit/Repatch.cpp:899
> +	   GCSafeConcurrentJSLocker locker(codeBlock->m_lock,
globalObject->vm().heap);

small nit: Maybe grab lock below the if and switches, since some cases return?