Closed Bug 1530937 Opened 6 years ago Closed 6 years ago

Refactor VMFunction interface

Categories

(Core :: JavaScript Engine: JIT, enhancement)

Product:
Core
Component:
JavaScript Engine: JIT
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Keywords: perf-alert)

Attachments

(18 files)

Bug 1530937 part 1 - Remove FunctionInfo Context template parameter. r?nbp!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 2 - Add new VMFunction mechanism and use it for some Baseline callVMs. r?nbp!,tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 3 - Convert more callVMs in BaselineCompiler. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 4 - Move pointer to C++ function out of VMFunctionData. r?nbp!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 5 - Convert more callVMs in BaselineCompiler. r?nbp!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 6 - Convert remaining callVMs in BaselineCompiler.cpp. r?nbp!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 7 - Move callVM and oolCallVM methods from CodeGeneratorShared to CodeGenerator. r?nbp!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 8 - Convert some Ion callVMs that already exist in VMFunctionList-inl.h. r?nbp!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 9 - Convert callVMs for Ion IC fallback functions. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 10 - Convert more callVMs in CodeGenerator.cpp. r?nbp!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 11 - Convert even more callVMs in CodeGenerator.cpp. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 12 - Convert remaining inline callVMs in CodeGenerator.cpp. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 13 - Convert Ion oolCallVMs and remove old CodeGenerator callVM overload. r?nbp!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 14 - Convert callVMs in CacheIR IC code. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 15 - Convert callVMs in BaselineIC code. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 16 - Convert tailCallVMs. r?tcampbell!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 17 - Remove now unused VMFunction code. r?nbp!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1530937 part 18 - Rename DoNew{Array,Object} to DoNew{Array,Object}Fallback and remove unused NewObject_WithTemplate IC kind. r?tcampbell!
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.

This was useful when we had ExclusiveContext and PJS ThreadSafeContext but now we
only use JSContext* so it's simpler to just use that.

We are doing this to:

  1. Eliminate (hundreds of) static constructors. These account for a significant
    fraction of all remaining static constructors in Gecko.

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

  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. We can
    use a Vector instead.

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

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:

https://godbolt.org/z/JkvC-q

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.

Flags: needinfo?(nfroyd)

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

https://godbolt.org/z/JkvC-q

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!

Flags: needinfo?(nfroyd)

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

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.

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

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.

Keywords: leave-open
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d629f7abeca0 part 1 - Remove FunctionInfo Context template parameter. r=nbp https://hg.mozilla.org/integration/autoland/rev/756272e36e32 part 2 - Add new VMFunction mechanism and use it for some Baseline callVMs. r=nbp,tcampbell

InitGetterSetterOperation had multiple overloads, I renamed them to
InitPropGetterSetterOperation and InitElemGetterSetterOperation.

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?

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*

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.

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

Does static_cast works? Which is supposed to be equivalent to the C cast and supposed to be supported.

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

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

Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7dc59aadcb9d part 3 - Convert more callVMs in BaselineCompiler. r=tcampbell

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

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.

Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/77b7364aabec part 4 - Move pointer to C++ function out of VMFunctionData. r=nbp

We had multiple overloads for js::Throw so I renamed the one we call here
for JSOP_THROW to js::ThrowOperation.

Also removes the old callVM overload so new code in BaselineCompiler must use
the new mechanism.

Depends on D22058

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.

Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a70132c4c444 part 5 - Convert more callVMs in BaselineCompiler. r=nbp https://hg.mozilla.org/integration/autoland/rev/4e8b8c1e0d19 part 6 - Convert remaining callVMs in BaselineCompiler.cpp. r=nbp

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

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.

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

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

Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/529da09b7f7e part 7 - Move callVM and oolCallVM methods from CodeGeneratorShared to CodeGenerator. r=nbp https://hg.mozilla.org/integration/autoland/rev/73efd5f3bfff part 8 - Convert some Ion callVMs that already exist in VMFunctionList-inl.h. r=nbp https://hg.mozilla.org/integration/autoland/rev/e50966a892bd part 9 - Convert callVMs for Ion IC fallback functions. r=tcampbell

(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 :/

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

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89599

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

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89599

It has been fixed, so GCC 9.1+ will reject the (void*) cast in all contexts.

Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5734a9a91531 part 10 - Convert more callVMs in CodeGenerator.cpp. r=nbp

  • 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

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.

Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b49300f6cc1f part 11 - Convert even more callVMs in CodeGenerator.cpp. r=tcampbell https://hg.mozilla.org/integration/autoland/rev/14778fd00dc5 part 12 - Convert remaining inline callVMs in CodeGenerator.cpp. r=tcampbell
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/16b947cb279a part 13 - Convert Ion oolCallVMs and remove old CodeGenerator callVM overload. r=nbp

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

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.

Tail calls have their own list/array/enum to improve type safety.

Depends on D23108

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

It's done. (Assuming parts 17 and 18 land without problems.)

Keywords: leave-open
Blocks: 1506763
Keywords: perf-alert
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: