Refactor VMFunction interface
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
(Keywords: perf-alert)
Attachments
(18 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
We want to rewrite it to:
(1) Eliminate (hundreds of) static constructors. These account for a significant fraction of all remaining static constructors in Gecko. Every time we add a new VMFunction we "risk" alerts from sheriffs, see bug 1525838.
(2) Use constexpr for VMFunction data. This is currently not possible with the linked list design but the new design will store all data in a constexpr array. This will save a few KB per process and is the right design.
(3) Make it easier to define a new VMFunction.
(4) Coalesce duplicate VMFunction copies in Baseline/Ion/ICs.
(5) Get rid of the (read-only) HashMap for the VMFunction => code lookup. The new design will use a Vector.
(6) Make it easier in the future to generate the wrappers at compile time.
I have patches to convert some functions to the new design based on the discussion in bug 1525838 and it seems to work well.
Assignee | ||
Comment 1•6 years ago
|
||
This was useful when we had ExclusiveContext and PJS ThreadSafeContext but now we
only use JSContext* so it's simpler to just use that.
Assignee | ||
Comment 2•6 years ago
|
||
We are doing this to:
-
Eliminate (hundreds of) static constructors. These account for a significant
fraction of all remaining static constructors in Gecko. -
Use constexpr for VMFunction data. This was not possible with the linked list
but the new design stores all data in a constexpr array. This will save a few
KB per process. -
Make it easier to define a new VMFunction.
-
Coalesce duplicate VMFunction copies in Baseline/Ion/ICs.
-
Get rid of the (read-only) HashMap for the VMFunction => code lookup. We can
use a Vector instead. -
Make it easier in the future to generate the wrappers at compile time.
This patch will let us incrementally convert the remaining VM functions. The
only thing not handled by this patch is support for the TailCall and
extraValuesToPop fields. We can do this when we convert the Baseline IC code
that uses these fields.
Once all VM functions have been converted we can remove and simplify more code.
Depends on D21331
Assignee | ||
Comment 3•6 years ago
|
||
Nathan, do you happen to know if it's valid to cast from function pointer to void* in constexpr code? Compilers don't like it with reinterpret_cast but a C-style cast works with both older and more recent versions of GCC and Clang:
I'm not sure if this is actually valid per spec but I'm unable to find a definitive answer - it would simplify this patch a lot though.
Comment 4•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3)
Nathan, do you happen to know if it's valid to cast from function pointer to void* in constexpr code? Compilers don't like it with reinterpret_cast but a C-style cast works with both older and more recent versions of GCC and Clang:
I'm not sure if this is actually valid per spec but I'm unable to find a definitive answer - it would simplify this patch a lot though.
The rules for constant expressions are defined here:
http://eel.is/c++draft/expr.const#4
and somewhat surprisingly, I don't see any mention of C-style casts in that list. So I think you are in the clear!
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #4)
The rules for constant expressions are defined here:
http://eel.is/c++draft/expr.const#4
and somewhat surprisingly, I don't see any mention of C-style casts in that list. So I think you are in the clear!
Thanks! I did see some comments online about the reinterpret_cast rule also applying to similar C-style casts but apparently that doesn't apply here. I'll just run with it: if this ever breaks we can change it back to what I had before without too much effort.
Comment 6•6 years ago
|
||
Tangentially related: (Baseline|Ion)CacheIRCompiler::(callVM|tailCallVM) still return |MOZ_MUST_USE bool|, even though they haven't been fallible since bug 1416572 made getVMWrapper infallible.
Once the coast is clear on this patch, I will probably fix that up.
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Iain Ireland [:iain] from comment #6)
Tangentially related: (Baseline|Ion)CacheIRCompiler::(callVM|tailCallVM) still return |MOZ_MUST_USE bool|, even though they haven't been fallible since bug 1416572 made getVMWrapper infallible.
Once the coast is clear on this patch, I will probably fix that up.
Good catch (I think getVMWrapper was infallible before bug 1416572 and null checks were redundant already).
After this lands I'll try to incrementally convert a number of VMFunctions each day so it will take a while until I get to the ones in the IC code. Feel free to change this before then.
Assignee | ||
Comment 8•6 years ago
|
||
One follow-up idea when all this is done: it might be possible to write a script to check all functions in VMFunctionList-inl.h are actually used in the codegen files (for each Foo in the list, search for a "callVM<Foo, ").
I'm not sure if it's super useful because when you remove/change the C++ symbol the compiler will complain anyway, so it would only help in situations where someone removes a callVM without removing (or modifying) the C++ function.
Still, it's nice to have the option if/when we're concerned about it.
Assignee | ||
Updated•6 years ago
|
Comment 10•6 years ago
|
||
bugherder |
Assignee | ||
Comment 11•6 years ago
|
||
InitGetterSetterOperation had multiple overloads, I renamed them to
InitPropGetterSetterOperation and InitElemGetterSetterOperation.
Comment 12•6 years ago
|
||
From IRC:
"mceier: firefox fails to compile with gcc 8.2.0 and 8.3.0: mozilla-central/js/src/jit/VMFunctions.cpp:98:1: error: a reinterpret_cast is not a constant expression"
gcc might be treating the C-style cast as a reinterpret_cast?
Comment 13•6 years ago
|
||
Okay, so if GCC allows to void*
cast with arrays, we can surely convince it that the void*
cast in a non-array is also okay and totally not a reinterpret_cast
→ https://godbolt.org/z/7E2fFS compiles just fine in GCC 8.2.0 and 8.3.0. *yuk*
Assignee | ||
Comment 14•6 years ago
|
||
Yeah it's not clear to me why we don't see this on godbolt with GCC 8.2 and 8.3...
I can post a patch to do what I had originally. I just wish I understood better what triggered it.
Comment 15•6 years ago
|
||
GCC appears to make a difference if the void*
result is directly stored in a member/variable or if it is first stored in an array (and than later retrieved from that array).
Comment 16•6 years ago
|
||
Does static_cast
works? Which is supposed to be equivalent to the C cast and supposed to be supported.
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to André Bargull [:anba] from comment #15)
GCC appears to make a difference if the
void*
result is directly stored in a member/variable or if it is first stored in an array (and than later retrieved from that array).
Oh I see, sorry I misread your message :)
(In reply to Nicolas B. Pierron [:nbp] from comment #16)
Does
static_cast
works? Which is supposed to be equivalent to the C cast and supposed to be supported.
It doesn't work.
It's still not completely clear to me if this should be valid per spec, but I can understand it being not allowed considering it's like a reinterpret_cast. Maybe I should just go with the safer design, but it's not as nice.
Comment 18•6 years ago
|
||
This still does break compilation with gcc 8.3:
js/src/jit/VMFunctions.cpp:98:1: in ‘constexpr’ expansion of ‘js::jit::VMFunctionDataHelper<bool ()(JSContext, js::jit::BaselineFrame*, unsigned char*, bool*)>(js::jit::DebugPrologue, ((const char*)"B
aselineDebugPrologue"), js::jit::PopValues(0))’
mozilla/js/src/jit/VMFunctions.cpp:98:1: error: a reinterpret_cast is not a constant expression
Comment 19•6 years ago
|
||
Assignee | ||
Comment 20•6 years ago
|
||
Thinking about it more, we should just put the function pointers in a separate "static const void*" array. That way VMFunctionData can stay constexpr and compilers are smart enough to treat a const array of pointers as constexpr. (We could still mark it constexpr but it's not necessary.)
Comment 21•6 years ago
|
||
bugherder |
Assignee | ||
Comment 22•6 years ago
|
||
GCC 8.2/8.3 doesn't like the (void*) trick in (some) constexpr contexts because
it treats it like a reinterpret_cast (which isn't allowed in constexpr). This
patch moves the function pointers into a separate const array so that
VMFunctionData can stay constexpr and relies on compilers being smart enough to
treat the function-pointer-array as constexpr.
Comment 23•6 years ago
|
||
Comment 24•6 years ago
|
||
bugherder |
Assignee | ||
Comment 25•6 years ago
|
||
We had multiple overloads for js::Throw so I renamed the one we call here
for JSOP_THROW to js::ThrowOperation.
Assignee | ||
Comment 26•6 years ago
|
||
Also removes the old callVM overload so new code in BaselineCompiler must use
the new mechanism.
Depends on D22058
Assignee | ||
Comment 27•6 years ago
|
||
These patches finish the work for BaselineCompiler. Later this week or next week I'll continue with Ion's CodeGenerator; a lot of the functions used there are now in VMFunctionList-inl.h so we can just reuse them.
Comment 28•6 years ago
|
||
Comment 29•6 years ago
|
||
bugherder |
Comment 30•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #22)
GCC 8.2/8.3 doesn't like the (void*) trick in (some) constexpr contexts because
it treats it like a reinterpret_cast (which isn't allowed in constexpr). This
patch moves the function pointers into a separate const array so that
VMFunctionData can stay constexpr and relies on compilers being smart enough to
treat the function-pointer-array as constexpr.
Thanks, worked for me!
Assignee | ||
Comment 31•6 years ago
|
||
They're only used in CodeGenerator.cpp so we can now move some of the helper
classes and templates from the header file to the cpp file.
Assignee | ||
Comment 32•6 years ago
|
||
Depends on D22278
Assignee | ||
Comment 33•6 years ago
|
||
Comment 34•6 years ago
|
||
Push from comment 10 won us some perf improvements:
== Change summary for alert #19707 (as of Thu, 28 Feb 2019 12:48:23 GMT) ==
Improvements:
4% raptor-assorted-dom-firefox linux64-pgo-qr opt 26.47 -> 25.39
4% raptor-assorted-dom-firefox linux64 pgo 26.47 -> 25.51
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19707
Comment 35•6 years ago
|
||
These were then canceled by comment 29:
== Change summary for alert #19786 (as of Tue, 05 Mar 2019 22:40:06 GMT) ==
Regressions:
6% raptor-assorted-dom-firefox linux64 pgo 25.16 -> 26.70
6% raptor-assorted-dom-firefox linux64-pgo-qr opt 25.08 -> 26.56
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19786
Comment 36•6 years ago
|
||
Assignee | ||
Comment 37•6 years ago
|
||
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #35)
These were then canceled by comment 29:
This confirms that assorted-dom is really bimodal and random (JS) changes can regress/improve it. We have seen this a few times now... I know it's annoying for perf sheriffs but I'm not sure what we can do about it :/
Assignee | ||
Comment 38•6 years ago
•
|
||
For what it's worth, yesterday I filed a GCC bug for their inconsistent constexpr behavior (thanks to André for tracking it down to array vs non-array).
Comment 39•6 years ago
|
||
bugherder |
Assignee | ||
Comment 40•6 years ago
|
||
Assignee | ||
Comment 41•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #38)
For what it's worth, yesterday I filed a GCC bug for their inconsistent constexpr behavior (thanks to André for tracking it down to array vs non-array).
It has been fixed, so GCC 9.1+ will reject the (void*) cast in all contexts.
Comment 42•6 years ago
|
||
Assignee | ||
Comment 43•6 years ago
|
||
Assignee | ||
Comment 44•6 years ago
|
||
-
Moves NewArrayWithGroup from CodeGenerator.cpp to builtin/Array.cpp
-
GetProperty has various overloads so I added GetValueProperty. I considered
renaming that GetProperty overload to GetValueProperty but there are quite
a lot of callers in VM code where GetProperty is probably closer to the spec
language. -
Ion called js::GetElement and js::CallElement which forwarded to GetElementOperation.
This was changed to call GetElementOperation directly (eliminates a VM wrapper).
Depends on D22677
Assignee | ||
Comment 45•6 years ago
|
||
When these patches land we can convert the oolCallVMs in CodeGenerator.cpp (there are about 50 so not too bad). After that we have the callVMs and tailCallVMs in IC code but there isn't a huge number of them compared to BaselineCompiler.cpp and CodeGenerator.cpp.
Comment 46•6 years ago
|
||
Comment 47•6 years ago
|
||
bugherder |
Assignee | ||
Comment 48•6 years ago
|
||
Comment 49•6 years ago
|
||
Comment 50•6 years ago
|
||
Yet another round of assorted-dom changes. I'll mark them as wontfix.
== Change summary for alert #19867 (as of Tue, 12 Mar 2019 03:16:07 GMT) ==
Regressions:
5% raptor-assorted-dom-firefox linux64 pgo 25.27 -> 26.59
5% raptor-assorted-dom-firefox linux64-pgo-qr opt 25.39 -> 26.56
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19867
Assignee | ||
Comment 51•6 years ago
|
||
Comment 52•6 years ago
|
||
bugherder |
Assignee | ||
Comment 53•6 years ago
|
||
Depends on D23107
Assignee | ||
Comment 54•6 years ago
|
||
With these patches the only VMFunctions left are for tail calls.
Initially I wanted to put the tail call functions in the same list (with some extra arguments), but it just occurred to me that if we put them in a separate list and use a separate enum class (TailCallVMFunctionId instead of VMFunctionId), the type system will make it impossible to use a tail-call-VMWrapper in a non-tail-callVM and vice versa. Pretty nice. Then we can also remove VMFunctionData::expectTailCall.
Assignee | ||
Comment 55•6 years ago
|
||
Tail calls have their own list/array/enum to improve type safety.
Depends on D23108
Assignee | ||
Comment 56•6 years ago
|
||
Depends on D23112
Assignee | ||
Comment 57•6 years ago
|
||
Depends on D23137
Assignee | ||
Comment 58•6 years ago
|
||
With the patches here I get "num_static_constructors opt: 98" on Linux64 Opt on Try. This was 102 or 103 without these changes.
Apparently GCC/Clang use a single huge static constructor per compilation unit (at least for all VMFunctions), with MSVC the drop would have been more dramatic :)
Assignee | ||
Comment 59•6 years ago
|
||
It's done. (Assuming parts 17 and 18 land without problems.)
Comment 60•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1e2091a92a1d
https://hg.mozilla.org/mozilla-central/rev/5aee21579936
https://hg.mozilla.org/mozilla-central/rev/6382f22140dd
https://hg.mozilla.org/mozilla-central/rev/3aa21d7302e7
https://hg.mozilla.org/mozilla-central/rev/aa1745f5137c
Comment 61•6 years ago
|
||
Comment 62•6 years ago
|
||
Updated•5 years ago
|
Description
•