Bug 93401 – Remove uses of ClassInfo in StrictEq and CompareEq in the DFG

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

Another incremental step in removing the dependence on ClassInfo pointers in object headers.

[Mark Hahnenberg]

Created attachment 157047 [details]
Patch

[Mark Hahnenberg]

Created attachment 157059 [details]
Patch

[Mark Hahnenberg]

Created attachment 157250 [details]
Patch

[Early Warning System Bot]

Comment on attachment 157250 [details]
Patch

Attachment 157250 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13458331

[Mark Hahnenberg]

Created attachment 157262 [details]
Patch

[Early Warning System Bot]

Comment on attachment 157262 [details]
Patch

Attachment 157262 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13452724

[Early Warning System Bot]

Comment on attachment 157262 [details]
Patch

Attachment 157262 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13457571

[Mark Hahnenberg]

Created attachment 157290 [details]
Patch

[Build Bot]

Comment on attachment 157290 [details]
Patch

Attachment 157290 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13461346

[Geoffrey Garen]

Comment on attachment 157290 [details]
Patch

/Volumes/Data/WebKit/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1620: warning: empty body in an if-statement
/Volumes/Data/WebKit/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1630: warning: empty body in an if-statement

Two more tweaks:

(1) When we detect a "masquerades" object, it should only masquerade in code from its same global object / document.

(2) The baseline JIT and interpreter should obey the same rules.

Please add a test case for the cross-document case, as well.

[Mark Hahnenberg]

Created attachment 160329 [details]
Patch

[Filip Pizlo]

Comment on attachment 160329 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:763
> +                forNode(node.child2()).filter(SpecCell | ~SpecString);

Isn't SpecCell | ~SpecString = everything?

As in:

SpecCell = the set of all possible cells
SpecString = the set of all possible strings.  note, SpecString is a subset of SpecCell
~SpecString = the set of all possible values minus the set of all strings.

Therefore, SpecCell | ~SpecString is the set of all possible values.

I'm pretty sure you meant something different here. ;-)

[Mark Hahnenberg]

(In reply to comment #12)
> (From update of attachment 160329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160329&action=review
> 
> > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:763
> > +                forNode(node.child2()).filter(SpecCell | ~SpecString);
> 
> Isn't SpecCell | ~SpecString = everything?
> 
> As in:
> 
> SpecCell = the set of all possible cells
> SpecString = the set of all possible strings.  note, SpecString is a subset of SpecCell
> ~SpecString = the set of all possible values minus the set of all strings.
> 
> Therefore, SpecCell | ~SpecString is the set of all possible values.
> 
> I'm pretty sure you meant something different here. ;-)

So maybe something like SpecCell & ~SpecString, if & is set intersection?

[Mark Hahnenberg]

Created attachment 160506 [details]
Patch

[Filip Pizlo]

Comment on attachment 160506 [details]
Patch

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

I buy your changes to the backend, but I don't think that your decision procedure for choosing when to speculate cell-that-is-not-string is right.  It will let through values that may be strings.

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:773
> +            if (left.shouldSpeculateCellOrOther() && right.shouldSpeculateCell()) {
>                  node.setCanExit(
> -                    !isArraySpeculation(forNode(node.child1()).m_type)
> -                    || !isArrayOrOtherSpeculation(forNode(node.child2()).m_type));
> -                forNode(node.child1()).filter(SpecArray);
> -                forNode(node.child2()).filter(SpecArray | SpecOther);
> +                    !isCellOrOtherSpeculation(forNode(node.child1()).m_type)
> +                    || !isCellSpeculation(forNode(node.child2()).m_type)
> +                    || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid());
> +                forNode(node.child1()).filter((SpecCell & ~SpecString) | SpecOther);
> +                forNode(node.child2()).filter((SpecCell & ~SpecString) | SpecOther);

I'm not sure I like how this case will be hit if a value can be either an object or a string (the previous if would not have fired since it only accepts strings), and then you'll speculate that the cell is not a string.  Can you change shouldSpeculateCellOrOther to shouldSpeculateNonStringCellOrOther?

Also, if we had proved that the thing is a string, then this will exit, but your setCanExit() call will say false.  As I've mentioned before, you can fix this by just doing node.setCanExit(true).  That is the safest thing to do here.

[Mark Hahnenberg]

Created attachment 160836 [details]
Patch

[Mark Hahnenberg]

Created attachment 160839 [details]
Patch

[Filip Pizlo]

Comment on attachment 160839 [details]
Patch

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

R=me but fix the thingies.  You know what to do!

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1194
> +            if (at(node.child1()).shouldSpeculateCell() && at(node.child2()).shouldSpeculateFinalObjectOrOther())

You missed a thingy.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1198
> +            else if (at(node.child1()).shouldSpeculateCell() && at(node.child2()).shouldSpeculateCell())

You missed a thingy.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2922
> +        if (at(node.child1()).shouldSpeculateCell() && at(node.child2()).shouldSpeculateFinalObjectOrOther()) {

You missed a thingy.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2927
> +        if (at(node.child1()).shouldSpeculateFinalObjectOrOther() && at(node.child2()).shouldSpeculateCell()) {

You missed a thingy.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2932
> +        if (at(node.child1()).shouldSpeculateCell() && at(node.child2()).shouldSpeculateCell()) {

Thingy!

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3068
> +    if (at(node.child1()).shouldSpeculateCell() && at(node.child2()).shouldSpeculateCell()) {

Thingy.

[Mark Hahnenberg]

Committed r127189: <http://trac.webkit.org/changeset/127189>