IonMonkey: Fast path for typeof x == y

NEW
Unassigned

Status

()

Core
JavaScript Engine
6 years ago
11 months ago

People

(Reporter: jandem, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ion:t])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
Like JM, we should have fast paths for these expressions:

typeof x == "..."
typeof x === ".."
Assignee: general → evilpies
So finally ;) I started looking into this. But I have the fear that this is going to introduce some kind of opcode fusing, so I don't like that. 

Probably the easiest way to solve this:
- Introduce MComapreS
- This way we don't have a box for the typeof and string
- Do the fusing (a bit like for LIsNullOrUndefined) in visitCompare so when we have lhs/rhs is typeof and lhs/rhs is string then do MTypeOfIs

Some other idea I had, introduce JSOP_TYPEOFIS, this way we dodge most of the above problems and also can handle the lhs/rhs switch easier in JM!

Some feedback?
Created attachment 628407 [details] [diff] [review]
WIP

After finding some stupid bug, where I wrongfully assumed that ops in Class is a pointer and not inside.

So yeah this has the simple case for typeof x == '..', where this typeof compare isn't inside an if block. Still needs this.

This also broke comparing with strings, so I need to refresh Bug 721438.

Updated

6 years ago
Blocks: 765980
Created attachment 639772 [details] [diff] [review]
WIP v2

This has TypeOfIsAndBranch. Still lacking tests.
Attachment #628407 - Attachment is obsolete: true
Created attachment 642014 [details] [diff] [review]
v1

I actually like this version of the patch. It's still very verbose, but I don't think there are a lot of ways to make it even simpler. The typeof x === "object" and typeof y === "function" look a little bit strange, because we implement a typeOf hook and our JSFunction objects have no Call hook!

Oh and tests, they are auto generated, so if you have some suggestions should be easy enough to add them.
Attachment #639772 - Attachment is obsolete: true
Attachment #642014 - Flags: review?(jdemooij)
(Reporter)

Comment 5

6 years ago
Comment on attachment 642014 [details] [diff] [review]
v1

Review of attachment 642014 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for taking this! r=me with nits below addressed.

The TypeOfIs and TypeOfIsAndBranch cases are very similar, but I agree that trying to combine them will probably make it worse.

::: js/src/ion/CodeGenerator.cpp
@@ +1823,5 @@
>      emitBranch(cond, lir->ifTrue(), lir->ifFalse());
>      return true;
>  }
>  
> +JSType

Nit: "static JSType".

@@ +1829,5 @@
> +{
> +    JSRuntime *rt = cx->runtime;
> +    JSType type = JSTYPE_LIMIT;
> +    if (EqualStrings(str, rt->atomState.typeAtoms[JSTYPE_STRING])) {
> +        type = JSTYPE_STRING;

Nit: you can remove the braces and return early:

if (EqualStrings(str, ...))
    return JSTYPE_STRING;
...
return JSTYPE_LIMIT;

@@ +1883,5 @@
> +        masm.branchTestNull(Assembler::Equal, tag, &isObject);
> +        masm.branchTestObject(Assembler::NotEqual, tag, &isntObject);
> +
> +        masm.unboxObject(value, output);
> +        masm.loadBaseShape(output, output);

This should be masm.loadObjClass. Please find out why the test didn't catch this though, in case there are other problems.

@@ +1912,5 @@
> +        Label isFunction, isntFunction, done;
> +        masm.branchTestObject(Assembler::NotEqual, tag, &isntFunction);
> +
> +        masm.unboxObject(value, output);
> +        masm.loadBaseShape(output, output);

masm.loadObjClass(output, output);

@@ +1935,5 @@
> +    } else if (type == JSTYPE_XML) {
> +        // Don't even really bother.
> +        cond = masm.testObject(Assembler::NotEqual, value);
> +        if (!bailoutIf(cond, lir->snapshot()))
> +            return false;

This should be Assembler::Equal, we want to bailout if the input is an object. Also add some tests to jit-test/tests/e4x/*, we have to live with it for a few more months.. ;)

@@ +1994,5 @@
> +
> +        masm.unboxObject(value, temp);
> +        masm.loadBaseShape(temp, temp);
> +
> +        masm.loadBaseShapeClass(temp, temp);

Nit: the previous two lines can be masm.loadObjClass(temp, temp);

@@ +2015,5 @@
> +        } else {
> +            // Swap branches.
> +            ifTrue = lir->ifFalse();
> +            ifFalse = lir->ifTrue();
> +        }

Nit: can we move the branch-swapping to the top of the function? It avoids duplicating this for object and function and may simplify the XML and LIMIT cases too.

@@ +2025,5 @@
> +
> +        masm.unboxObject(value, temp);
> +        masm.loadBaseShape(temp, temp);
> +
> +        masm.loadBaseShapeClass(temp, temp);

Nit: 

masm.unboxObject(value, temp);
masm.loadObjClass(temp, temp);

@@ +2039,5 @@
> +    } else if (type == JSTYPE_XML) {
> +        // Don't even really bother.
> +        cond = masm.testObject(Assembler::NotEqual, value);
> +        if (!bailoutIf(cond, lir->snapshot()))
> +            return false;

Nit: Assembler::Equal here too.

::: js/src/ion/LIR-Common.h
@@ +1075,5 @@
>  
> +class LTypeOfIs : public LInstructionHelper<1, BOX_PIECES, 0>
> +{
> +    JSOp op_;
> +    const js::Value &v_;

You can remove the & here, storing a reference seems dangerous.

@@ +1100,5 @@
> +
> +class LTypeOfIsAndBranch : public LInstructionHelper<0, BOX_PIECES, 1>
> +{
> +    JSOp jsop_;
> +    const js::Value &v_;

Same here.

::: js/src/ion/Lowering.cpp
@@ +411,5 @@
>  
> +
> +        if (comp->specialization() == MIRType_String) {
> +            // We are only allowed to reorder if it's really going to match.
> +            if (IsTypeOfCompare(left, right) || IsTypeOfCompare(right, left)) {

Nit: if the specialization is "string", swapping the arguments is not observable, so it's okay to just swap and then call IsTypeOfCompare.

::: js/src/jit-test/tests/ion/typeof-is.js
@@ +44,5 @@
> +function isUndefined2(x) {
> +    if ("undefined" === typeof x)
> +        return true;
> +    return false;
> +}

Nit: would be good to add isBanana(x) or something here too.

@@ +46,5 @@
> +        return true;
> +    return false;
> +}
> +
> +for (var i = 0; i < 10000; i++) {

Nit: considering the large loop body, 100 or so may be better (tests run with --ion-eager / --no-jm).
Attachment #642014 - Flags: review?(jdemooij) → review+
Created attachment 642759 [details] [diff] [review]
v2

Thank you for spotting this stupid bug :) Just attached the updated version.
Attachment #642014 - Attachment is obsolete: true
Attachment #642759 - Flags: review+
Depends on: 774510
Oh well I just realized this patch has a pretty big gapping hole and for a lot of code this is now actually slower than before!

Because for some strange reason we don't eliminate the LTypeOfV, but still do LTypeOfIs.
We never set emitAtUse for the MTypeOf, because for some reason the instruction has an second usage namely MBox. I have no idea where this is coming from, I can't see it in iongraph after all. Maybe something with resumepoints?
Whiteboard: [ion:t]
Created attachment 647538 [details] [diff] [review]
v3

So I changed the approach somewhat. We know do the fusing in IonBuilder. This also allows us to stop doing the StringCompare in CodeGenerator. There is one small problem:

eg. for code like this

>t = typeof obj
>if (t == 'object')
>  x.push(t)  

We are not allowed to just remove the typeof and such. The problem is at the point of the compare the typeof still has an use count of 1.
Attachment #642759 - Attachment is obsolete: true
I am not working on this.
Assignee: evilpies → general
(Reporter)

Updated

5 years ago
Blocks: 606648
For everyone who wants to try this. I think it might be easier to transform the two patterns typeof x == 'object' and object == typeof x in bytecode.
s/object/"object"
(Assignee)

Updated

4 years ago
Assignee: general → nobody
Some day this should really be done. I would imagine integrating this with the "filter types at branches" infrastructure might be useful.
You need to log in before you can comment on or make changes to this bug.