LIRGenerator/CodeGenerator snapshot code should be faster

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: jandem, Assigned: nbp)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Background Ion compilation threads spend a lot of time encoding snapshots, the LIRGenerator and CodeGenerator bits combined are sometimes more than 25%.

A lot of this is SnapshotWriter::add doing HashMap lookups, but there are also some hot virtual calls to MResumePoint methods, some non-inlined calls, etc.
(Reporter)

Comment 1

2 years ago
nbp volunteered :)
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 2

2 years ago
This patch devirtualize calls to numOperands and getOperand of MResumePoint,
which allows them to be inlined.

This is improves OptimizeMIR by ~1.2%.
Attachment #8895462 - Flags: review?(jdemooij)
(Assignee)

Comment 3

2 years ago
Change the code of the RValueAllocation hash() and operator==() functions,
to reduce the time spent under the hash functions.

This new method relies on the fact that the full content of the
RValueAllocation is initialized (adding static_asserts to ensure that), and
use the verbatim content of it to generate the hash and the comparison
without having to interpret the content of the RValueAllocations.

This patch improves OptimizeMIR by ~5%.
Attachment #8895465 - Flags: review?(jdemooij)
(Assignee)

Comment 5

2 years ago
With this current set of patches, I cannot spot any obvious slow down in any of the function, which can easily be removed without changing the design.

I thought of caching the mapping between MIR & LAllocations to RValueAllocations from CodeGeneratorShared::encodeAllocation, but this while this might be possible, this would not solve the issue that we have while building the snapshot under the LIR, nor issues with MResumePoints.

Maybe we should think of larger refactoring plans of our LSnapshot and MResumePoint to introduce LDeltaSnapshot from the register allocator, and MDeltaResumePoint from IonBuilder.  This would be a larger project than this one.
(Reporter)

Comment 6

2 years ago
Comment on attachment 8895462 [details] [diff] [review]
part 1 - IonMonkey: Devirtualize MResumePoint::getOperand function calls.

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

Nice.
Attachment #8895462 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 7

2 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> This is improves OptimizeMIR by ~1.2%.

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> This patch improves OptimizeMIR by ~5%.

Do you mean GenerateLIR or CompileBackEnd?
(Reporter)

Comment 8

2 years ago
Comment on attachment 8895465 [details] [diff] [review]
part 2 - IonMonkey: Simplify RValueAllocation hash function.

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

::: js/src/jit/Snapshots.h
@@ +191,5 @@
>      RValueAllocation(Mode mode, Payload a1)
>        : mode_(mode),
>          arg1_(a1)
>      {
> +        arg2_.index = 0;

I think it would be simpler and safer to wrap the union in a struct with a constructor (or add a constructor to the union, not sure if all compilers support that now). Like this:

http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/js/src/jit/MIR.h#1552-1567

Then you don't need the explicit index = 0 assignments.
Attachment #8895465 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 9

2 years ago
Comment on attachment 8895466 [details] [diff] [review]
part 3 - IonMonkey: CodeGeneratorShared::encodeAllocation: guards debug-only loop in ifdef DEBUG.

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

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +406,5 @@
>          MNode** it = recoverInfo->begin();
>          MNode** end = recoverInfo->end();
>          while (it != end && mir != *it) {
>              ++it;
>              ++index;

|index| is used after the #ifdef DEBUG code?
Attachment #8895466 - Flags: review?(jdemooij)
(Assignee)

Comment 10

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #9)
> ::: js/src/jit/shared/CodeGenerator-shared.cpp
> @@ +406,5 @@
> >          MNode** it = recoverInfo->begin();
> >          MNode** end = recoverInfo->end();
> >          while (it != end && mir != *it) {
> >              ++it;
> >              ++index;
> 
> |index| is used after the #ifdef DEBUG code?

Oh, good catch, I will make a test case for it then.


(In reply to Jan de Mooij [:jandem] from comment #7)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > This is improves OptimizeMIR by ~1.2%.
> 
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > This patch improves OptimizeMIR by ~5%.
> 
> Do you mean GenerateLIR or CompileBackEnd?

I meant CompileBackEnd.
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 11

2 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> (In reply to Jan de Mooij [:jandem] from comment #9)
> > |index| is used after the #ifdef DEBUG code?
> 
> Oh, good catch, I will make a test case for it then.

This is already caught by ion/dec/with-rinstructions.js test case, but only with optimized builds.

Comment 12

2 years ago
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea704d8cd779
part 1 - IonMonkey: Devirtualize MResumePoint::getOperand function calls. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/68444a862226
part 2 - IonMonkey: Simplify RValueAllocation hash function. r=jandem

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea704d8cd779
https://hg.mozilla.org/mozilla-central/rev/68444a862226
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee: nobody → nicolas.b.pierron
You need to log in before you can comment on or make changes to this bug.