| Summary: | Change behavior of MasqueradesAsUndefined to better accommodate DFG changes | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, dglazkov, fpizlo, ggaren, haraken, japhet, ossy, rniwa, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | 94144, 94147 | ||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Mark Hahnenberg
2012-08-13 12:54:21 PDT
Created attachment 158088 [details]
Patch
Comment on attachment 158088 [details] Patch Attachment 158088 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13491297 Comment on attachment 158088 [details] Patch Attachment 158088 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13481943 Comment on attachment 158088 [details] Patch Attachment 158088 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13490250 Comment on attachment 158088 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158088&action=review r- because the builders are angry. Please check my performance comment below, as well. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:645 > + JITCompiler::Jump isMasqueradesAsUndefined = m_jit.branchTest8(JITCompiler::NonZero, JITCompiler::Address(resultPayloadGPR, Structure::typeInfoFlagsOffset()), JITCompiler::TrustedImm32(MasqueradesAsUndefined)); > > + if (invert) > + m_jit.move(TrustedImm32(0), resultPayloadGPR); > + else > + m_jit.move(TrustedImm32(1), resultPayloadGPR); > + > + JITCompiler::Jump notMasqueradesAsUndefined = m_jit.jump(); > + > + isMasqueradesAsUndefined.link(&m_jit); > + GPRTemporary localGlobalObject(this); > + GPRTemporary remoteGlobalObject(this); > + GPRReg localGlobalObjectGPR = localGlobalObject.gpr(); > + GPRReg remoteGlobalObjectGPR = remoteGlobalObject.gpr(); > + m_jit.move(JITCompiler::TrustedImmPtr(m_jit.graph().globalObjectFor(m_jit.graph()[operand].codeOrigin)), localGlobalObjectGPR); > + m_jit.loadPtr(JITCompiler::Address(resultPayloadGPR, Structure::globalObjectOffset()), remoteGlobalObjectGPR); > + m_jit.compare32(invert ? JITCompiler::NotEqual : JITCompiler::Equal, localGlobalObjectGPR, remoteGlobalObjectGPR, resultPayloadGPR); > + This looks like it would be a performance regression. Do you have benchmark results for this patch? I thought our plan was: (a) Don't test masqueradesAsUndefined at all if you global object has not produced document.all (b) Watchpoint equality comparisons, and fire the watchpoint if your global object does produce document.all I believe that plan would be a speedup over what we have now. > Source/JavaScriptCore/runtime/Structure.h:513 > + inline bool Structure::isMasqueradesAsUndefined(JSGlobalObject* lexicalGlobalObject) > + { > + return typeInfo().masqueradesAsUndefined() && globalObject() == lexicalGlobalObject; > + } I think just plain "masqueradesAsUndefined" would be a better name here. Created attachment 158150 [details]
Patch
Comment on attachment 158150 [details] Patch Attachment 158150 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13494290 Comment on attachment 158150 [details] Patch Attachment 158150 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13492326 New failing tests: fast/js/document-all-between-frames.html Created attachment 158175 [details]
Archive of layout-test-results from gce-cr-linux-05
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 158150 [details] Patch Attachment 158150 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13490366 Created attachment 158448 [details]
Patch
I'm not sure how to fix the EFL build failure. The compiler claims it's an incorrect use of JSValue::toBoolean without the ExecState* argument, but I don't see any incorrect uses of JSValue::toBoolean() anywhere in the entire Source directory. Any EFL people out there that can give me a hint? Comment on attachment 158448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158448&action=review r=me with one repeated comment below. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:616 > + m_jit.graph().globalObjectFor(m_jit.graph()[operand].codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint()); You should only do this if the watchpoint is still valid. If it's invalid, adding to it doesn't make sense. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:670 > + m_jit.graph().globalObjectFor(m_jit.graph()[operand].codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint()); Ditto. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3817 > + m_jit.graph().globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint()); Ditto. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:581 > + m_jit.graph().globalObjectFor(m_jit.graph()[operand].codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint()); Ditto. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:635 > + m_jit.graph().globalObjectFor(m_jit.graph()[operand].codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint()); Ditto. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:3808 > + m_jit.graph().globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint()); Ditto. Comment on attachment 158448 [details] Patch Attachment 158448 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13502484 New failing tests: fast/js/document-all-between-frames.html Created attachment 158457 [details]
Archive of layout-test-results from gce-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 158448 [details] Patch Attachment 158448 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13500501 Committed r125687: <http://trac.webkit.org/changeset/125687> (In reply to comment #17) > Committed r125687: <http://trac.webkit.org/changeset/125687> It broke the whole world: https://bugs.webkit.org/show_bug.cgi?id=94144 Could you check it, please? Re-opened since this is blocked by 94147 Created attachment 160247 [details]
Patch
Comment on attachment 160247 [details]
Patch
r=me
Comment on attachment 160247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160247&action=review > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:788 > - || !isFinalObjectOrOtherSpeculation(forNode(node.child2()).m_type)); > + || !isFinalObjectOrOtherSpeculation(forNode(node.child2()).m_type) > + || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid()); > forNode(node.child1()).filter(SpecFinalObject); > forNode(node.child2()).filter(SpecFinalObject | SpecOther); > break; > } else if (right.shouldSpeculateFinalObject() && left.shouldSpeculateFinalObjectOrOther()) { > node.setCanExit( > !isFinalObjectOrOtherSpeculation(forNode(node.child1()).m_type) > - || !isFinalObjectSpeculation(forNode(node.child2()).m_type)); > + || !isFinalObjectSpeculation(forNode(node.child2()).m_type) > + || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid()); > forNode(node.child1()).filter(SpecFinalObject | SpecOther); > forNode(node.child2()).filter(SpecFinalObject); > break; > } else if (left.shouldSpeculateArray() && right.shouldSpeculateArrayOrOther()) { > node.setCanExit( > !isArraySpeculation(forNode(node.child1()).m_type) > - || !isArrayOrOtherSpeculation(forNode(node.child2()).m_type)); > + || !isArrayOrOtherSpeculation(forNode(node.child2()).m_type) > + || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid()); > forNode(node.child1()).filter(SpecArray); > forNode(node.child2()).filter(SpecArray | SpecOther); > break; > } else if (right.shouldSpeculateArray() && left.shouldSpeculateArrayOrOther()) { > node.setCanExit( > !isArrayOrOtherSpeculation(forNode(node.child1()).m_type) > - || !isArraySpeculation(forNode(node.child2()).m_type)); > + || !isArraySpeculation(forNode(node.child2()).m_type) > + || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid()); Why did you change this? Your patch doesn't change any of the backend code that corresponds to this! > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:796 > + node.setCanExit( > + !isAnySpeculation(forNode(node.child1()).m_type) > + || !isAnySpeculation(forNode(node.child2()).m_type) > + || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid()); isAnySpeculation returns true all the time. So this code is very strange. > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:798 > + forNode(node.child1()).filter(SpecTop); > + forNode(node.child2()).filter(SpecTop); Please remove this; filtering on TOP does nothing. Created attachment 160269 [details]
Oops
Committed r126494: <http://trac.webkit.org/changeset/126494> It appears that this broke EFL builds: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815 http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815/steps/compile-webkit/logs/stdio (In reply to comment #25) > It appears that this broke EFL builds: > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815 > > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815/steps/compile-webkit/logs/stdio I mentioned earlier in the bug that I could not figure out how to fix these build errors. I see no mention of the functions that claim they don't compile anywhere in the Source directory. (In reply to comment #26) > (In reply to comment #25) > > It appears that this broke EFL builds: > > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815 > > > > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815/steps/compile-webkit/logs/stdio > > I mentioned earlier in the bug that I could not figure out how to fix these build errors. I see no mention of the functions that claim they don't compile anywhere in the Source directory. It's in derived source. (In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #25) > > > It appears that this broke EFL builds: > > > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815 > > > > > > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815/steps/compile-webkit/logs/stdio > > > > I mentioned earlier in the bug that I could not figure out how to fix these build errors. I see no mention of the functions that claim they don't compile anywhere in the Source directory. > > It's in derived source. I think Mark's point is that he already changed the code generators to use the right function signature. So EFL may be doing something strange here. (In reply to comment #28) > I think Mark's point is that he already changed the code generators to use the right function signature. > > So EFL may be doing something strange here. Maybe it just need a kick? It appears that touching the idl file worked: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug (In reply to comment #30) > It appears that touching the idl file worked: > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug Sweet, thanks Ryosuke! |