Closed Bug 1416592 Opened 2 years ago Closed 2 years ago

Deduplicate VMFunction wrappers

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
We have some duplicate VMFunction definitions, for instance when we call the same C++ function from Baseline or Ion. Right now we will generate 2 identical wrappers, but it just occurred to me we can easily avoid this if we add a custom VMFunction hash policy that compares the wrapped function (and other VMFunction fields) instead of the VMFunction* pointer.

The attached patch does this. I added some logging and we have 37 duplicates (14%). That means we now emit 226 wrappers instead of 263.

This patch applies on top of bug 1416572.
Attachment #8927678 - Flags: review?(nicolas.b.pierron)
Component: JavaScript Engine → JavaScript Engine: JIT
Comment on attachment 8927678 [details] [diff] [review]
Patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +6307,5 @@
>  
>  typedef bool (*InitElemGetterSetterFn)(JSContext*, jsbytecode*, HandleObject, HandleValue,
>                                         HandleObject);
>  static const VMFunction InitElemGetterSetterInfo =
> +    FunctionInfo<InitElemGetterSetterFn>(InitGetterSetterOperation, "InitElemGetterSetterOperation");

(I had to change the names of some functions to match the names we use in BaselineCompiler.cpp - we now assert the strings are equal in VMFunction::match).
(In reply to Jan de Mooij [:jandem] from comment #0)
> […] we add a custom VMFunction hash policy that compares the wrapped function
> (and other VMFunction fields) instead of the VMFunction* pointer.

Why checking other fields? The wrapped function pointer should be enough to uniquely identify a given function with a given signature.
Comment on attachment 8927678 [details] [diff] [review]
Patch

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

::: js/src/jit/VMFunctions.h
@@ +285,5 @@
> +    }
> +    static bool match(const VMFunction* f1, const VMFunction* f2) {
> +        if (f1->wrapped != f2->wrapped ||
> +            f1->extraValuesToPop != f2->extraValuesToPop ||
> +            f1->expectTailCall != f2->expectTailCall)

nit: (see comment 2)
Attachment #8927678 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Why checking other fields? The wrapped function pointer should be enough to
> uniquely identify a given function with a given signature.

We generate different wrapper code if the wrapper is called as tail call or if we have to pop extra Values from the stack - these values are passed to VMFunction. However I don't know if we actually have C++ functions that are called both as non-tail call and tail call... I could add asserts for these 2 fields if you want and then we can see if they fail.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Jan de Mooij [:jandem] from comment #4)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > Why checking other fields? The wrapped function pointer should be enough to
> > uniquely identify a given function with a given signature.
> 
> We generate different wrapper code if the wrapper is called as tail call or
> if we have to pop extra Values from the stack - these values are passed to
> VMFunction. However I don't know if we actually have C++ functions that are
> called both as non-tail call and tail call... I could add asserts for these
> 2 fields if you want and then we can see if they fail.

As will certainly get these assertion during JitRuntime init, adding these assertion with a comment saying that we hope for none sounds like the best option.

Otherwise r+ with the minimal set of fields.
Flags: needinfo?(nicolas.b.pierron)
js::Throw has a tail call here (and non-tail calls elsewhere):

https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/js/src/jit/BaselineIC.cpp#4599

I'll add an assert for extraValuesToPop.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa5eb7e2ed9
Deduplicate VMFunction wrapper code. r=nbp
https://hg.mozilla.org/mozilla-central/rev/3aa5eb7e2ed9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.