LIRGenerator/CodeGenerator snapshot code should be faster

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: jandem, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

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

11 months ago
nbp volunteered :)
Flags: needinfo?(nicolas.b.pierron)
Created attachment 8895462 [details] [diff] [review]
part 1 - IonMonkey: Devirtualize MResumePoint::getOperand function calls.

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)
Created attachment 8895465 [details] [diff] [review]
part 2 - IonMonkey: Simplify RValueAllocation hash function.

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)
Created attachment 8895466 [details] [diff] [review]
part 3 - IonMonkey: CodeGeneratorShared::encodeAllocation: guards debug-only loop in ifdef DEBUG.

Removing debug-only code.
Attachment #8895466 - Flags: review?(jdemooij)
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

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

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

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

11 months 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)
(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)
(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

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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea704d8cd779
https://hg.mozilla.org/mozilla-central/rev/68444a862226
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.