1416592 - Deduplicate VMFunction wrappers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 1

[Jan de Mooij [:jandem]]

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).

Comment 2

[Nicolas B. Pierron [:nbp]]

(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 3

[Nicolas B. Pierron [:nbp]]

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)

Comment 4

[Jan de Mooij [:jandem]]

(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.

Comment 5

[Nicolas B. Pierron [:nbp]]

(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.

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa5eb7e2ed9
Deduplicate VMFunction wrapper code. r=nbp

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/3aa5eb7e2ed9