93884 – Change behavior of MasqueradesAsUndefined to better accommodate DFG changes

RESOLVED FIXED 93884

Change behavior of MasqueradesAsUndefined to better accommodate DFG changes

https://bugs.webkit.org/show_bug.cgi?id=93884

Summary Change behavior of MasqueradesAsUndefined to better accommodate DFG changes

Mark Hahnenberg

Reported 2012-08-13 12:54:21 PDT

With some upcoming changes to the DFG to remove uses of ClassInfo, we will be changing the behavior of MasqueradesAsUndefined. In order to make this change consistent across all of our execution engines, we will make this change to MasqueradesAsUndefined as a separate patch. After this patch, MasqueradesAsUndefined objects will only masquerade as undefined in their original context (i.e. their original JSGlobalObject). For example, if an object that masquerades as undefined in frame A is passed to frame B, it will not masquerade as undefined within frame B, but it will continue to masquerade in frame A.

Attachments
Patch (256.99 KB, patch) 2012-08-13 13:26 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Patch (268.38 KB, patch) 2012-08-13 17:11 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-05 (527.24 KB, application/zip) 2012-08-13 18:07 PDT, WebKit Review Bot	no flags	Details
Patch (274.43 KB, patch) 2012-08-14 16:50 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-02 (389.00 KB, application/zip) 2012-08-14 17:42 PDT, WebKit Review Bot	no flags	Details
Patch (278.07 KB, patch) 2012-08-23 14:29 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Oops (273.36 KB, patch) 2012-08-23 15:48 PDT, Mark Hahnenberg	fpizlo: review+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Mark Hahnenberg

Comment 1 2012-08-13 13:26:34 PDT

Created attachment 158088 [details] Patch

Early Warning System Bot

Comment 2 2012-08-13 14:14:53 PDT

Comment on attachment 158088 [details] Patch Attachment 158088 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13491297

Early Warning System Bot

Comment 3 2012-08-13 14:23:33 PDT

Comment on attachment 158088 [details] Patch Attachment 158088 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13481943

Gyuyoung Kim

Comment 4 2012-08-13 14:24:24 PDT

Comment on attachment 158088 [details] Patch Attachment 158088 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13490250

Geoffrey Garen

Comment 5 2012-08-13 14:36:12 PDT

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.

Mark Hahnenberg

Comment 6 2012-08-13 17:11:28 PDT

Created attachment 158150 [details] Patch

Gyuyoung Kim

Comment 7 2012-08-13 17:42:25 PDT

Comment on attachment 158150 [details] Patch Attachment 158150 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13494290

WebKit Review Bot

Comment 8 2012-08-13 18:06:58 PDT

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

WebKit Review Bot

Comment 9 2012-08-13 18:07:03 PDT

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

Build Bot

Comment 10 2012-08-13 20:07:16 PDT

Comment on attachment 158150 [details] Patch Attachment 158150 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13490366

Mark Hahnenberg

Comment 11 2012-08-14 16:50:14 PDT

Created attachment 158448 [details] Patch

Mark Hahnenberg

Comment 12 2012-08-14 16:51:55 PDT

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?

Geoffrey Garen

Comment 13 2012-08-14 17:13:23 PDT

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.

WebKit Review Bot

Comment 14 2012-08-14 17:41:58 PDT

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

WebKit Review Bot

Comment 15 2012-08-14 17:42:02 PDT

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

Gyuyoung Kim

Comment 16 2012-08-14 17:50:34 PDT

Comment on attachment 158448 [details] Patch Attachment 158448 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13500501

Mark Hahnenberg

Comment 17 2012-08-15 11:32:57 PDT

Committed r125687: <http://trac.webkit.org/changeset/125687>

Csaba Osztrogonác

Comment 18 2012-08-15 13:55:58 PDT

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

WebKit Review Bot

Comment 19 2012-08-15 14:28:23 PDT

Re-opened since this is blocked by 94147

Mark Hahnenberg

Comment 20 2012-08-23 14:29:04 PDT

Created attachment 160247 [details] Patch

Geoffrey Garen

Comment 21 2012-08-23 14:32:26 PDT

Comment on attachment 160247 [details] Patch r=me

Filip Pizlo

Comment 22 2012-08-23 14:57:35 PDT

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.

Mark Hahnenberg

Comment 23 2012-08-23 15:48:21 PDT

Created attachment 160269 [details] Oops

Mark Hahnenberg

Comment 24 2012-08-23 16:00:07 PDT

Committed r126494: <http://trac.webkit.org/changeset/126494>

Ryosuke Niwa

Comment 25 2012-08-23 17:39:03 PDT

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

Mark Hahnenberg

Comment 26 2012-08-23 17:41:57 PDT

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

Ryosuke Niwa

Comment 27 2012-08-23 17:44:41 PDT

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

Filip Pizlo

Comment 28 2012-08-23 17:46:04 PDT

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

Ryosuke Niwa

Comment 29 2012-08-23 17:48:51 PDT

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

Ryosuke Niwa

Comment 30 2012-08-23 18:10:51 PDT

It appears that touching the idl file worked: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug

Mark Hahnenberg

Comment 31 2012-08-23 19:04:43 PDT

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component JavaScriptCore

Assignee

Mark Hahnenberg

Reported

2012-08-13 12:54 PDT

Modified

2012-08-23 19:04 PDT History

CC List

9 users Show

URL

Keywords

Depends on

94144 94147

Blocks

Dependencies

tree graph