1388014 - LIRGenerator/CodeGenerator snapshot code should be faster

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

nbp volunteered :)

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: 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%.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: 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%.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - IonMonkey: CodeGeneratorShared::encodeAllocation: guards debug-only loop in ifdef DEBUG.

Removing debug-only code.

Comment 5

[Nicolas B. Pierron [:nbp]]

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.

Comment 6

[Jan de Mooij [:jandem]]

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

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

Nice.

Comment 7

[Jan de Mooij [:jandem]]

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

Comment 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[Jan de Mooij [:jandem]]

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?

Comment 10

[Nicolas B. Pierron [:nbp]]

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

Comment 11

[Nicolas B. Pierron [:nbp]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ea704d8cd779
https://hg.mozilla.org/mozilla-central/rev/68444a862226