Bug 93884 - Change behavior of MasqueradesAsUndefined to better accommodate DFG changes
Summary: Change behavior of MasqueradesAsUndefined to better accommodate DFG changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on: 94144 94147
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-13 12:54 PDT by Mark Hahnenberg
Modified: 2012-08-23 19:04 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 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.
Comment 1 Mark Hahnenberg 2012-08-13 13:26:34 PDT
Created attachment 158088 [details]
Patch
Comment 2 Early Warning System Bot 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
Comment 3 Early Warning System Bot 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
Comment 4 Gyuyoung Kim 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
Comment 5 Geoffrey Garen 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.
Comment 6 Mark Hahnenberg 2012-08-13 17:11:28 PDT
Created attachment 158150 [details]
Patch
Comment 7 Gyuyoung Kim 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
Comment 8 WebKit Review Bot 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
Comment 9 WebKit Review Bot 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
Comment 10 Build Bot 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
Comment 11 Mark Hahnenberg 2012-08-14 16:50:14 PDT
Created attachment 158448 [details]
Patch
Comment 12 Mark Hahnenberg 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?
Comment 13 Geoffrey Garen 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.
Comment 14 WebKit Review Bot 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
Comment 15 WebKit Review Bot 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
Comment 16 Gyuyoung Kim 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
Comment 17 Mark Hahnenberg 2012-08-15 11:32:57 PDT
Committed r125687: <http://trac.webkit.org/changeset/125687>
Comment 18 Csaba Osztrogonác 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?
Comment 19 WebKit Review Bot 2012-08-15 14:28:23 PDT
Re-opened since this is blocked by 94147
Comment 20 Mark Hahnenberg 2012-08-23 14:29:04 PDT
Created attachment 160247 [details]
Patch
Comment 21 Geoffrey Garen 2012-08-23 14:32:26 PDT
Comment on attachment 160247 [details]
Patch

r=me
Comment 22 Filip Pizlo 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.
Comment 23 Mark Hahnenberg 2012-08-23 15:48:21 PDT
Created attachment 160269 [details]
Oops
Comment 24 Mark Hahnenberg 2012-08-23 16:00:07 PDT
Committed r126494: <http://trac.webkit.org/changeset/126494>
Comment 26 Mark Hahnenberg 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.
Comment 27 Ryosuke Niwa 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.
Comment 28 Filip Pizlo 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.
Comment 29 Ryosuke Niwa 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?
Comment 30 Ryosuke Niwa 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
Comment 31 Mark Hahnenberg 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!