Bug 135593 – Using +s to convert a string to a number badly confuses the DFG

• Component: JavaScriptCore
• Status: NEW

[Mark Hahnenberg]

I was writing a toy Huffman encoder/decoder in JS, and I was comparing the performance of JSC to nodejs. I ran the benchmark by hooking it into the octane harness. nodejs crushes JSC on this particular benchmark (by like 10x). I was curious and ran the profiler, and I noticed a very simple function was exiting a ton. Here's the source of the function:

function (i) { return +i; }

I'll attach the profiler output separately.

[Mark Hahnenberg]

Created attachment 236001 [details]
profiler output

[Mark Hahnenberg]

Created attachment 236002 [details]
graph dump

[Mark Hahnenberg]

Created attachment 236003 [details]
huffman benchmark

[Mark Hahnenberg]

Created attachment 236004 [details]
huffman benchmark

[Mark Hahnenberg]

It looks like during the prediction injection phase we  notice that the argument is a StringIdent, but the Phantom continues to assume that it's a number which causes us to exit constantly.

[Mark Hahnenberg]

Created attachment 236052 [details]
Patch

[Filip Pizlo]

Comment on attachment 236052 [details]
Patch

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

Almost there

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:309
> +    case ValueToNumber: {

Can we call it ToNumber?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:318
> +        if (child && child.isInt32()) {
> +            setConstant(node, child);
> +            break;
> +        }
> +        if (child && child.isNumber()) {
> +            setConstant(node, jsDoubleNumber(child.asNumber()));
> +            break;
> +        }

I feel like this should just be:

if (child && child.isNumber()) {
    setConstant(node, child);
    break;
}

> Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:362
> +            node->child1()->mergeFlags(flags);

I would mask off NodeBytecodeUsesAsOther, as in: node->child1()->mergeFlags(flags & ~NodeNytecodeUsesAsOther)

> Source/JavaScriptCore/dfg/DFGClobberize.h:351
> +        read(MiscFields);

It reads world.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1153
> +        case ValueToNumber:

This needs to have the identity stuff.  And probably some stuff to use BooleanToNumber.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:479
> +            changed |= setPrediction(SpecFullNumber);

This needs to be more granular.  If the input is already an int or already a number then you should preserve the original input's type.  Literally you want something like:

if (node->child1()->prediction() & SpecCell)
    changed |= setPrediction(SpecFullNumber);
else {
    SpeculatedType type = node->child1()->prediction();
    if (type & SpecOther) {
        type &= ~SpecOther;
        type |= SpecInt32 | SpecDoubleNaN;
    }
    if (type & SpecBoolean) {
        type &= ~SpecBoolean; 
        type |= SpecInt32;
    }
    mergePredicstion(type);
}

[Mark Hahnenberg]

Created attachment 236135 [details]
Patch

[Mark Hahnenberg]

Created attachment 236304 [details]
Patch

[Filip Pizlo]

Comment on attachment 236304 [details]
Patch

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

R=me.  It would be slightly better if this also included the FTL implementation of ToNumber.  Also, a test would be cool.  I'm fine with the FTL implementation being a separate patch.  As a test, maybe you could just land your huffman thing, if you haven't already?  Even better, land both huffman, and a microbenchmark for +s.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1283
> +    void fixupToNumber(Node* node)
> +    {
> +        if (node->child1()->shouldSpeculateInt32()) {
> +            fixIntOrBooleanEdge(node->child1());
> +            node->convertToIdentity();
> +            return;
> +        }
> +
> +        if (node->child1()->shouldSpeculateMachineInt()) {
> +            fixEdge<Int52RepUse>(node->child1());
> +            node->convertToIdentity();
> +            node->setResult(NodeResultInt52);
> +            return;
> +        }
> +
> +        if (node->child1()->shouldSpeculateNumber()) {
> +            fixDoubleOrBooleanEdge(node->child1());
> +            node->convertToIdentity();
> +            node->setResult(NodeResultDouble);
> +            return;
> +        }
> +    }
> +

You only call this in one place so you might as well inline it. :-)

fixupToPrimitive() had a method because we would call it in more than one place.  Hopefully that's still the case, or else someone should probably inline it.