Convert WebIDL bindings to something CallArgs-like

RESOLVED FIXED in mozilla24

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla24
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 2 obsolete attachments)

12.55 KB, patch
peterv
: review+
Details | Diff | Splinter Review
14.13 KB, patch
peterv
: review+
Details | Diff | Splinter Review
21.54 KB, patch
peterv
: review+
Details | Diff | Splinter Review
1.01 KB, patch
terrence
: review+
Details | Diff | Splinter Review
52.35 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.96 KB, patch
Details | Diff | Splinter Review
46.33 KB, patch
jandem
: review+
peterv
: review+
Details | Diff | Splinter Review
4.80 KB, patch
jandem
: review+
Details | Diff | Splinter Review
7.88 KB, patch
jandem
: review+
Details | Diff | Splinter Review
This uses the infrastructure from bug 877216.
Created attachment 755533 [details] [diff] [review]
Codegen changes WIP

Still needs jit changes to pass the right things in
Created attachment 755973 [details] [diff] [review]
part 1.  Convert WebIDL bindings to using something CallArgs-like.

Peter, would you review the codegen changes?

Jan, would you review the JS engine changes?

As far as the JS engine changes go, please pay careful attention to the visitCallDOMNative bits and ionframe bits in terms of the following:

1)  Can we add more asserts somehow to make this whole setup safer?
2)  Can we make better use of registers by having another scratch reg to work with?
    At first glance no, but please double-check that!
3)  Is there a cleaner way to compute the argv value to push on the stack?

In terms of performance this makes a no-op DOM call slower by about 0.5ns on my machine, presumably for two reasons.  First of all, the C++ implementation has to return JS::UndefinedValue().  This used to involve just writing it to *vp, but now it has to effectively be written to *(args->array), which means an extra pointer-chase.  Second, the actual jit codegen is a bit different, obviously.  The relevant change is the old setup did this in the JIT:

  ;; movePtr(StackPointer, argVp)
  movq %rsp, %r8

whereas the new setup does this:

  ;; computeEffectiveAddress(
  ;;   Address(StackPointer, sizeof(uintptr_t) + 2*sizeof(Value)),
  ;;   argArgs)
  leaq 0x18(%rsp), %rcx
  ;; Push(argArgs);
  push %rcx
  ;; movePtr(StackPointer, argArgs);
  movq %rsp, %rcx

And I'm on x86-64 so having to pass one less argument to the ABI call doesn't matter: they're all getting passed in registers anyway.  It's possible that on x86-32 there is no performance impact and that in a non-microbenchmark the reduced register pressure helps or something...
Attachment #755973 - Flags: review?(peterv)
Attachment #755973 - Flags: review?(jdemooij)
Attachment #755533 - Attachment is obsolete: true
Created attachment 755976 [details] [diff] [review]
part 2.  Eliminate uses of ${valPtr} in bindings conversions.
Attachment #755976 - Flags: review?(peterv)
Created attachment 755977 [details] [diff] [review]
part 3.  Rename valMutableHandle to mutableVal.

This is pure search-and-replace
Attachment #755977 - Flags: review?(peterv)
Created attachment 755981 [details] [diff] [review]
part 4.  Eliminate uses of ${valHandle} in binding conversions and make ${val} be a handle.
Attachment #755981 - Flags: review?(peterv)
Created attachment 755982 [details] [diff] [review]
part 5.  Add a set() method to Rooted.
Attachment #755982 - Flags: review?(terrence)
Created attachment 755990 [details] [diff] [review]
part 6.  Replace ${jsvalPtr} with a MutableHandle ${jsvalHandle}.   a little unfortunate that we need both ${jsvalHandle} and

I'm sorry this is so big; I couldn't find a good way to split this up
Attachment #755990 - Flags: review?(peterv)
Blocks: 580070
Whiteboard: [need jit changes] → [need review]
Comment on attachment 755982 [details] [diff] [review]
part 5.  Add a set() method to Rooted.

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

r=me

::: js/public/RootingAPI.h
@@ +578,5 @@
>          ptr = value;
>          return ptr;
>      }
>  
> +    void set(T value) {

Why not |const T &value|?
Attachment #755982 - Flags: review?(terrence) → review+
> Why not |const T &value|?

I cribbed from MutableHandle.

But a possibly-good reason for not doing that is that this code:

  Rooted<JSObject*> rooted(cx);
  JSObject* obj = GetSomeObject();
  rooted.set(obj);

would end up with an unsafe reference hazard if the set() argument is a const T&....
Comment on attachment 755973 [details] [diff] [review]
part 1.  Convert WebIDL bindings to using something CallArgs-like.

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

Looks great, but would be good to take another look once the comments below are addressed.

::: js/src/ion/CodeGenerator.cpp
@@ +1416,5 @@
>      Register obj = masm.extractObject(Address(StackPointer, 0), argObj);
>      JS_ASSERT(obj == 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].

Nit: remove the last part: "The StackPointer..." since that movePtr is gone now.

@@ +1417,5 @@
>      JS_ASSERT(obj == 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));

It seems like we can do

masm.computeEffectiveAddress(Address(StackPointer, 2 * sizeof(Value)), argArgs);

here to get vp + 2, if we no longer need argArgs for the argc below.

@@ +1428,2 @@
>      // Push argument into what will become the IonExitFrame
> +    masm.Push(argArgs);

masm.Push(Imm32(call->numStackArgs());  We can get rid of the extra move now because we no longer have to pass argc as argument.

@@ +1438,5 @@
> +    JS_STATIC_ASSERT(JSJitMethodCallArgs::offsetOfArgc() ==
> +                     IonDOMMethodExitFrameLayout::offsetOfArgcFromArgv());
> +    masm.computeEffectiveAddress(Address(StackPointer,
> +                                         sizeof(uintptr_t) + 2*sizeof(Value)),
> +                                 argArgs);

And remove this call.

::: js/src/ion/shared/IonFrames-x86-shared.h
@@ +433,5 @@
>      IonExitFrameLayout exit_;
>      // This must be the last thing pushed, so as to stay common with
>      // IonDOMExitFrameLayout.
>      JSObject *thisObj_;
> +    Value* argv_;

Nit: Value *argv_; (same for the ARM version).
Attachment #755973 - Flags: review?(jdemooij)
> Nit: remove the last part: "The StackPointer..." since that movePtr is gone now.

That's not a comment about the movePtr.  That's a comment about where StackPointer points to after this Push() call, and I believe it's still correct, though I rephrased it a bit to make that clearer.

Made the rest of the changes.  Very good catches!
Created attachment 756307 [details] [diff] [review]
Interdiff addressing Jan's review comments
Created attachment 756308 [details] [diff] [review]
Part 1 updated to Jan's review comments
Attachment #756308 - Flags: review?(peterv)
Attachment #756308 - Flags: review?(jdemooij)
Attachment #755973 - Attachment is obsolete: true
Attachment #755973 - Flags: review?(peterv)
Comment on attachment 756308 [details] [diff] [review]
Part 1 updated to Jan's review comments

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

r=me
Attachment #756308 - Flags: review?(jdemooij) → review+
Attachment #756308 - Flags: review?(peterv) → review+
Attachment #755976 - Flags: review?(peterv) → review+
Attachment #755977 - Flags: review?(peterv) → review+
Attachment #755981 - Flags: review?(peterv) → review+
Attachment #755990 - Flags: review?(peterv) → review+
Thanks for the reviews!

   https://hg.mozilla.org/integration/mozilla-inbound/rev/1907d472e5d4
   https://hg.mozilla.org/integration/mozilla-inbound/rev/840183b434d2
   https://hg.mozilla.org/integration/mozilla-inbound/rev/98ec2d7ed87f
   https://hg.mozilla.org/integration/mozilla-inbound/rev/20f763a69b1f
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f047d17cdb78
   https://hg.mozilla.org/integration/mozilla-inbound/rev/3c733ec01b14
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla24
Created attachment 760062 [details] [diff] [review]
bustage fix.  Deal with the fact that Value has 8-byte alignment.
Attachment #760062 - Flags: review?(jdemooij)
I pushed the bustage fix as https://hg.mozilla.org/integration/mozilla-inbound/rev/39a59be2a4cc for the moment...
Created attachment 760064 [details] [diff] [review]
Another fix: things weren't constexpr enough for msvc
Attachment #760064 - Flags: review?(jdemooij)
And pushed that second bustage fix as http://hg.mozilla.org/integration/mozilla-inbound/rev/697190293f4e
Blocks: 796850
https://hg.mozilla.org/mozilla-central/rev/1907d472e5d4
https://hg.mozilla.org/mozilla-central/rev/840183b434d2
https://hg.mozilla.org/mozilla-central/rev/98ec2d7ed87f
https://hg.mozilla.org/mozilla-central/rev/20f763a69b1f
https://hg.mozilla.org/mozilla-central/rev/f047d17cdb78
https://hg.mozilla.org/mozilla-central/rev/3c733ec01b14
https://hg.mozilla.org/mozilla-central/rev/39a59be2a4cc
https://hg.mozilla.org/mozilla-central/rev/697190293f4e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Attachment #760062 - Flags: review?(jdemooij) → review+

Updated

5 years ago
Attachment #760064 - Flags: review?(jdemooij) → review+
You need to log in before you can comment on or make changes to this bug.