Last Comment Bug 773549 - IonMonkey: Handle JSOP_CALL on DOM methods.
: IonMonkey: Handle JSOP_CALL on DOM methods.
Status: RESOLVED FIXED
[ion:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Eric Faust [:efaust]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 773548 822340 847119
Blocks: 747290
  Show dependency treegraph
 
Reported: 2012-07-12 23:59 PDT by Eric Faust [:efaust]
Modified: 2013-03-04 19:59 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First take on methods (174.13 KB, patch)
2012-07-14 17:13 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review
DOM Methods from JSOP_CALL (17.41 KB, patch)
2012-07-19 17:48 PDT, Eric Faust [:efaust]
sstangl: review+
Details | Diff | Splinter Review

Description Eric Faust [:efaust] 2012-07-12 23:59:07 PDT
Generate native calls to DOM specialized methods, rather than a NativeCall to the top half
Comment 1 Eric Faust [:efaust] 2012-07-14 17:13:53 PDT
Created attachment 642291 [details] [diff] [review]
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] 2012-07-19 17:48:43 PDT
Created attachment 644099 [details] [diff] [review]
DOM Methods from JSOP_CALL
Comment 3 Sean Stangl [:sstangl] 2012-07-26 16:15:42 PDT
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] 2012-08-08 17:06:21 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/b2382c3c24ce

Note You need to log in before you can comment on or make changes to this bug.