Closed Bug 877281 Opened 6 years ago Closed 6 years ago

Convert WebIDL bindings to something CallArgs-like

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 2 obsolete files)

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.
Attached patch Codegen changes WIP (obsolete) — Splinter Review
Still needs jit changes to pass the right things in
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
This is pure search-and-replace
Attachment #755977 - Flags: review?(peterv)
I'm sorry this is so big; I couldn't find a good way to split this up
Attachment #755990 - Flags: review?(peterv)
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!
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+
Blocks: 796850
Attachment #760062 - Flags: review?(jdemooij) → review+
Attachment #760064 - Flags: review?(jdemooij) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.