Summary: | [JSC] Polymorphic PutByVal | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, pvollan, saam, tzagallo, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2021-08-17 23:31:57 PDT
Created attachment 435749 [details]
Patch
Created attachment 435789 [details]
Patch
Created attachment 435804 [details]
Patch
Created attachment 435816 [details]
Patch
Created attachment 435827 [details]
Patch
Created attachment 435828 [details]
Patch
Created attachment 435846 [details]
Patch
Created attachment 435856 [details]
Patch
Created attachment 436051 [details]
Patch
Created attachment 436069 [details]
Patch
Created attachment 436070 [details]
Patch
Created attachment 436071 [details]
Patch
Created attachment 436154 [details]
Patch
Created attachment 436158 [details]
Patch
Created attachment 436163 [details]
Patch
Created attachment 436174 [details]
Patch
Created attachment 436176 [details]
Patch
Created attachment 436177 [details]
Patch
Created attachment 436260 [details]
Patch
Created attachment 436294 [details]
Patch
Created attachment 436296 [details]
Patch
Created attachment 436337 [details]
Patch
Created attachment 436338 [details]
Patch
Created attachment 436341 [details]
Patch
Created attachment 436423 [details]
Patch
Created attachment 436429 [details]
Patch
Comment on attachment 436429 [details] 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? Created attachment 436464 [details]
Patch
Patch for landing
Comment on attachment 436429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436429&action=review >> Source/JavaScriptCore/bytecode/AccessCase.cpp:1068 >> + if (stubInfo.m_arrayProfileGPR != InvalidGPRReg) > > assert it's invalidGPR? Changed. >> Source/JavaScriptCore/bytecode/AccessCase.cpp:1170 >> + allocator.lock(stubInfo.m_arrayProfileGPR); > > assert it's invalidGPR? Changed. >> Source/JavaScriptCore/bytecode/AccessCase.cpp:1257 >> + allocator.lock(stubInfo.m_arrayProfileGPR); > > assert it's invalidGPR? Changed. >> 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. Changed to failAndRepatch. >> Source/JavaScriptCore/bytecode/AccessCase.cpp:1505 >> + failAndIgnore.append(jit.branchIfNotInt32(valueRegs)); > > ditto regarding repatching vs not repatching. Changed to failAndRepatch. >> Source/JavaScriptCore/bytecode/AccessCase.cpp:1573 >> + state.failAndIgnore.append(jit.branchIfNotInt32(valueRegs)); > > ditto about repatching or ignoring. Changed to failAndRepatch. >> Source/JavaScriptCore/bytecode/AccessCase.cpp:1582 >> + state.failAndIgnore.append(jit.branchIfNotNumber(valueRegs.payloadGPR())); > > ditto Changed to failAndRepatch. >> 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? Discussed with Saam at Slack. We will continue using failAndRepatch since OOB TypedArray access case should soon get the slow path. >> Source/JavaScriptCore/bytecode/AccessCase.cpp:2434 >> + allocator.lock(stubInfo.m_arrayProfileGPR); > > Assert invalidGPR? Changed. >> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp:145 >> + allocator.lock(stubInfo.m_arrayProfileGPR); > > assert is invalidGPR? Changed. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:6475 >> + tryCompileAsPutByOffset = true; > > let's rename this to "compiledAsPutPrivateNameById" Changed. >> 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? Fixed! Committed r281615 (240971@main): <https://commits.webkit.org/240971@main> *** Bug 203866 has been marked as a duplicate of this bug. *** |