773549 - IonMonkey: Handle JSOP_CALL on DOM methods.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Generate native calls to DOM specialized methods, rather than a NativeCall to the top half

Comment 1

[Eric Faust [:efaust]]

Attachment: First take on methods

When applied to Ionmonkey tip, this patch drops the following testcase:

 <canvas id="canvas" width="300" height="300">  
      Sorry, your browser doesn't support the &lt;canvas&gt; element.  
 </canvas>
  <script>
    var canvas = document.getElementById("canvas");
    var xhr = canvas.getContext("2d");
    var count = 10000000;
    var start = new Date();
    for (var i = 0; i < count; ++i)
      xhr.testBindings();
    var end = new Date();
    alert(end - start);
  </script>

drops from 117ms to 86ms. I expect the rest is unavoidable function call overhead, but there might be some minor inefficiencies in the jitcode.

This with infallible void method testBindings which takes no args.

Comment 2

[Eric Faust [:efaust]]

Attachment: DOM Methods from JSOP_CALL

Comment 3

[Sean Stangl [:sstangl]]

Comment on attachment 644099 [details] [diff] [review]
DOM Methods from JSOP_CALL

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

LGTM.

::: js/src/ion/CodeGenerator.cpp
@@ +605,5 @@
> +    masm.movePtr(StackPointer, argObj);
> +
> +    // Push a Value containing the callee object: natives are allowed to access their callee before
> +    // setitng the return value. The StackPointer is moved to &vp[0].
> +    masm.Push(ObjectValue(*target));

Not sure whether this is necessary for DOM natives, but I suppose it doesn't hurt much.

@@ +608,5 @@
> +    // setitng the return value. The StackPointer is moved to &vp[0].
> +    masm.Push(ObjectValue(*target));
> +    masm.movePtr(StackPointer, argVp);
> +
> +    // Use argArgc to hold the |this| obj for the private unbox

// Use argArgc as scratch.

@@ +650,5 @@
> +
> +    // Handle exception case.
> +    {
> +        masm.bind(&exception);
> +        masm.handleException();

Nicolas has a patch in Bug 775167 that will need to handle this if this lands first. If that's the case, would you mind making a note in that bug?

::: js/src/ion/IonBuilder.cpp
@@ +4001,3 @@
>          // For property accesses which may be on many objects, we just need to
>          // find a prototype common to all the objects; if that prototype
>          // has the property, the access will not be on a missing property.

Changing this comment to read "has the singleton property" would be arguably clearer.

@@ +4005,5 @@
> +        for (unsigned i = 0; i < types->getObjectCount(); i++) {
> +            types::TypeObject *object = types->getTypeObject(i);
> +            if (!object) {
> +                // Try to get it through the singleton.
> +                JSObject *curObj = types->getSingleObject(i);

Comment on holes in getTypeObject().

@@ +5104,5 @@
>      return true;
>  }
>  
> +static bool
> +TestDOMTypes(JSContext *cx, types::TypeSet *inTypes)

TestAreKnownDOMTypes?

@@ +5151,5 @@
> +
> +            curType = curObj->getType(cx);
> +        }
> +
> +        bool wasntDOM = types::TypeSet::HasObjectFlags(cx, curType,

DebugOnly<bool>. It may also want a comment above it stating that HasObjectFlags() sets a freeze, because that's extremely non-obvious, especially since hasAnyFlags() *doesn't* set a freeze.

The types::OBJECT_FLAG_NON_DOM looks like it can be on the same line. Wraps at 99 characters.

@@ +5220,5 @@
> +            current->push(known);
> +            if (singleton->isFunction()) {
> +                RootedFunction singletonFunc(cx, singleton->toFunction());
> +                if (TestDOMTypes(cx, unaryTypes.inTypes) &&
> +                    TestShouldDOMCall(cx, true, unaryTypes.inTypes, singletonFunc)) {

nit: { on next line

::: js/src/ion/LIR-Common.h
@@ +714,5 @@
> +        JS_ASSERT(mir()->numStackArgs() >= 1);
> +        return mir()->numStackArgs() - 1;
> +    }
> +    uint32 numActualArgs() const {
> +        JS_NOT_REACHED("Do I actually need this?");

Probably not. numStackArgs() is just max(expected, passed). If you don't call it, it's not necessary.

::: js/src/ion/Lowering.cpp
@@ +259,5 @@
> +                                                     tempFixed(CallTempReg0),
> +                                                     tempFixed(CallTempReg1),
> +                                                     tempFixed(CallTempReg2),
> +                                                     tempFixed(CallTempReg3),
> +                                                     tempFixed(CallTempReg4));

It almost feels silly declaring these registers explicitly, since they're guaranteed to be unused anyway. Oh well.

Comment 4

[Eric Faust [:efaust]]

https://hg.mozilla.org/projects/ionmonkey/rev/b2382c3c24ce