1105651 - Better type information on untyped BinaryArith

Status: RESOLVED INVALID

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Hannes Verschore [:h4writer]]

In Browsermark DOM Create Source the function JQuery.acceptData is hot. Which has a function like:

function test(elem) {
    var nodeType = +elem.nodeType || 1
    return nodeType !== 1 && nodeType !== 9
}

Now "+elem.nodeType" is translated into "MMul GetProperty 1", which goes through a vmcall since the GetProperty is an object. So the result type of MMul is MIRType_Value. Actually we can improve this for jsop_binary, since the result is always MIRType_Double. 

Beefing up the types improved a small testcase from 1.7s to 1.5s. Haven't tested it on the original benchmark.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Wait, why MMul?  This should be a unary plus, not a binary anything...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Also, nodeType knows it returns JSVAL_TYPE_INT32 if we manage to hit the Ion fast-path here, so I'd think the unary '+' would be detected as the no-op it is in this case.  Assuming "elem" is a DOM node, of course.

Comment 4

[Hannes Verschore [:h4writer]]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #2)
> Wait, why MMul?  This should be a unary plus, not a binary anything...

Because the unary plus was implemented as a "x * 1". It has the same side effects and doesn't require a new MIR.

(In reply to Please do not ask for reviews for a bit [:bz] from comment #3)
> Also, nodeType knows it returns JSVAL_TYPE_INT32 if we manage to hit the Ion
> fast-path here, so I'd think the unary '+' would be detected as the no-op it
> is in this case.  Assuming "elem" is a DOM node, of course.

We have a MGetPropertyCache for the "nodeType", so there is no fast-path here :(.


It might be good to figure out why we have a MGetPropertyCache here. Nevertheless the patch should also be considered to land, since it also improves the engine for other cases.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> It might be good to figure out why we have a MGetPropertyCache here.

Agreed that should be a separate bug.  I was going to file it, but I can't find the code referenced here in the DOM create source bits.  Which version of jQuery are you seeing that use?  I see 2.0.3, which has acceptData looking like this:

  Data.accepts = function( owner ) {
	// Accepts only:
	//  - Node
	//    - Node.ELEMENT_NODE
	//    - Node.DOCUMENT_NODE
	//  - Object
	//    - Any
	return owner.nodeType ?
		owner.nodeType === 1 || owner.nodeType === 9 : true;
  };

(which might explain why we get an MGetPropertyCache, if it's passing in non-node objects).

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #5)
> > It might be good to figure out why we have a MGetPropertyCache here.
> 
> Agreed that should be a separate bug.  I was going to file it, but I can't
> find the code referenced here in the DOM create source bits.  Which version
> of jQuery are you seeing that use?  I see 2.0.3, which has acceptData
> looking like this:

Yes, I use 2.0.3. I think that is the version included in Browsermark 2.1

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  Then is the function in comment 0 after Ion has done some transformations on the MIR?

Anyway, filed 1106100.

Comment 8

[Hannes Verschore [:h4writer]]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #7)
> OK.  Then is the function in comment 0 after Ion has done some
> transformations on the MIR?

Yes, IonMonkey translates "+X" to "X * 1" when creating the graph.

Comment 9

[Boris Zbarsky [:bzbarsky]]

No, I meant where does the +X come from?

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8529633 [details] [diff] [review]
Patch

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

::: js/src/jit/IonBuilder.cpp
@@ +4185,5 @@
> +    // When the operation cannot get optimized a VMCall is used. Hence the
> +    // result type is MIRType_Value. Though all operations definitely return
> +    // MIRType_Double. Unbox here for better types.
> +    if (ins->type() == MIRType_Value) {
> +        MUnbox *unbox = MUnbox::New(alloc(), current->pop(), MIRType_Double, MUnbox::Infallible);

MAdd can also return strings, it's used if we don't have enough type information to use MConcat.

Also, using MIRType_Double instead of MIRType_Int32 could lead to performance problems down the road. How about either:

(1) Continue to use MAdd/MMul/etc with MIRType_Value, but give it a resultTypeSet of {int32,double} (and for MAdd also add 'string' if one of the operands is a string or object).

(2) Use Baseline IC info to see which result types we have seen and unbox based on that, fallible or infallible.

I like (1) because it's simpler, would it work for this case?

Comment 11

[Jan de Mooij [:jandem]]

Hannes is this still on your radar?

Comment 12

[André Bargull [:anba]]

Old, IonBuilder related bug -> no longer valid with Warp.