Refactor VMFunction interface

RESOLVED FIXED in Firefox 67

Status

()

enhancement
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(18 attachments)

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
Assignee

Description

4 months ago

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

4 months 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

4 months ago

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

Assignee

Comment 3

4 months 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:

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

Comment 5

4 months 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.

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

4 months 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

4 months 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

4 months ago
Keywords: leave-open

Comment 9

4 months ago
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
Assignee

Comment 11

4 months ago

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_casthttps://godbolt.org/z/7E2fFS compiles just fine in GCC 8.2.0 and 8.3.0. *yuk*

Assignee

Comment 14

4 months 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.

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.

Assignee

Comment 17

4 months 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

4 months 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

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

Comment 20

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

Assignee

Comment 22

4 months 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

4 months ago
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
Assignee

Comment 25

4 months 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

4 months ago

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

Depends on D22058

Assignee

Comment 27

4 months 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

4 months ago
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

Comment 30

4 months 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

4 months 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.

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

Comment 36

4 months ago
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
Assignee

Comment 37

4 months 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

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

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

Assignee

Comment 41

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

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.

Comment 42

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

Comment 44

4 months 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

4 months 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

4 months ago
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

Comment 49

4 months ago
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

Assignee

Comment 54

4 months 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

4 months ago

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

Depends on D23108

Assignee

Comment 58

4 months 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

4 months ago

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

Keywords: leave-open
Assignee

Updated

4 months ago
Blocks: 1506763
You need to log in before you can comment on or make changes to this bug.