962555 - Clean-up: Refactor Snapshot::Slot to use more than 16 registers on ARM.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently our ARM backend is limited to 16 registers because of the way we encode the slots.  We could manipulate up to 32 doubles or 16 doubles + 32 floats at the same time in registers. (Without mentioning upcoming quad registers for SIMD)

We need to clean-up this code as our current bit-packing is not explicit.  I suggest to make structures to highlight the bit packing with bit-fields.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: [Part 1] Extract SnapshotReader::Slot

I already made a similar patch as part of Bug 878503, which got an r+, the main difference here is that I also moved the read & write functions to be Slot members. (part 2) The reason is that I intend to add a unit test which ensure that we are able to decode any encoded slot, and obtain the same Slot.

The current move duplicates a bit of the logic which appears in the CodeGeneratorShared::encodeSlots function. (part 3) I intend to extend the Mode such as we do not have to execute more conditions to write these slots down.

I did not renamed Location to SlotLocation as suggested as we already have a SlotLocation which is used in Baseline.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: [Part 1] Extract SnapshotReader::Slot

Delta:
 - Add missing files.
 - Add test cases/assertions to cover/prevent handled/non-handled usages.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8366617 [details] [diff] [review]
[Part 1] Extract SnapshotReader::Slot

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

::: js/src/jit/Slot.cpp
@@ +135,5 @@
> +                return TypedSlot(type, Register::FromCode(reg2));
> +            return TypedSlot(type, reader.readSigned());
> +        }
> +
> +        Slot slot(UNTYPED);

already-nit: I removed this line locally.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: [part 2] Add multiple UNTYPED modes to prevent duplicating conditions.

This patch should remove the overhead added in the previous patch by adding a specialized UNTYPED mode.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8366617 [details] [diff] [review]
[Part 1] Extract SnapshotReader::Slot

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

Decided on IRC that SnapshotReader.h and SnapshotWriter.h get merged into Snapshot.h and will contain Slot.h. Also the name of Slot is changed to RValueAllocation. I'm clearing review for now. But it was decided that Nicholas will post a follow-up that changes this state to what is requested. I'll add r? flag again when reviewing that patch.

::: js/src/jit/Slot.cpp
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * vim: set ts=8 sts=4 et sw=4 tw=99:
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

We decided to move this into Snapshot.h/Snapshot.cpp

@@ +12,5 @@
> +using namespace js;
> +using namespace js::jit;
> +
> +// [slot] Information on how to reify a js::Value from an Ion frame. The
> +//         first byte is split as thus:

This information is now in two places, in Snapshots.cpp and here.

@@ +24,5 @@
> +//         JSVAL_TYPE_INT32:
> +//         JSVAL_TYPE_OBJECT:
> +//         JSVAL_TYPE_BOOLEAN:
> +//         JSVAL_TYPE_STRING:
> +//              If "reg" is InvalidReg1, this byte is followed by a [vws]

Isn't this "InvalidReg". I don't find InvalidReg1

::: js/src/jit/Snapshots.cpp
@@ +263,5 @@
>      // Place a sentinel for asserting on the other end.
>  #ifdef DEBUG
>      writer_.writeSigned(-1);
>  #endif
>      

pre-existing whitespace

::: js/src/jsapi-tests/testJitSlot.cpp
@@ +3,5 @@
> + */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Awesome to have tests for this!

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8366750 [details] [diff] [review]
[part 2] Add multiple UNTYPED modes to prevent duplicating conditions.

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

lgtm

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: [part 3] Merge Headers and rename Slot to RValueAllocation.

Merge SnapshotReader.h, SnapshotWriter.h and Slot.h in Snapshot.h
Rename Slot to RValueAllocation, as well as some variables referring to Slots.
  `- Rename stackSlot() to stackOffset(), as this is the current meaning of it.

Note: At the same time, I removed the track-snapshot code which is no longer used since we landed baseline.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Note: At the same time, I removed the track-snapshot code which is no longer
> used since we landed baseline.

Would it be a lot of work to keep it? I've a patch in my queue that adds the spewBailingFrom() call to BaselineBailouts.cpp and it's very useful to see the LIR/MIR instruction that bailed when analyzing benchmarks.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> (In reply to Nicolas B. Pierron [:nbp] from comment #7)
> > Note: At the same time, I removed the track-snapshot code which is no longer
> > used since we landed baseline.
> 
> Would it be a lot of work to keep it? I've a patch in my queue that adds the
> spewBailingFrom() call to BaselineBailouts.cpp and it's very useful to see
> the LIR/MIR instruction that bailed when analyzing benchmarks.

This should be easy to add it back.

Comment 10

[Hannes Verschore [:h4writer]]

Comment on attachment 8367986 [details] [diff] [review]
[part 3] Merge Headers and rename Slot to RValueAllocation.

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

Is there a reason you named it Snapshot.h and not Snapshots.h? A typo?

::: js/src/jit/IonFrameIterator.h
@@ +246,5 @@
>      uintptr_t fromStack(const Location &loc);
>  
> +    Value allocValue(const RValueAllocation &alloc);
> +    bool allocReadable(const RValueAllocation &alloc);
> +    void warnUnreadableAllocation();

Hmmm, I was reading this as: "Allocate a Value". I think we should rename every "alloc" in this patch to "allocation". That way there shouldn't be any confusion

::: js/src/jit/Snapshots.cpp
@@ +29,5 @@
>  //   [ptr] A fixed-size pointer.
>  //   [vwu] A variable-width unsigned integer.
>  //   [vws] A variable-width signed integer.
>  //    [u8] An 8-bit unsigned integer.
> +//  [rva*] Information on how to find a js::Value in an Ion frame. The

adjust the comment to include RValueAllocation reference:
a RValueAllocation. Contains information ...

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #10)
> Review of attachment 8367986 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there a reason you named it Snapshot.h and not Snapshots.h? A typo?

Indeed, this is a typo.

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: [part 4] Merge Headers and rename Slot to RValueAllocation.

Comment 13

[Nicolas B. Pierron [:nbp]]

Attachment: [part 3] Use RValueAllocation mode to infer if this is a stack or register location.

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 8370703 [details] [diff] [review]
[part 4] Merge Headers and rename Slot to RValueAllocation.

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

Mentioned over irc, but this only gets r+ if snapIter.spewBailingFrom() doesn't get removed! This is used during debugging and will probably get added back soonish.

::: js/src/jit/Snapshots.cpp
@@ +115,5 @@
> +
> +    switch (type) {
> +      case JSVAL_TYPE_DOUBLE:
> +        if (code < MIN_REG_FIELD_ESC)
> +            return Double(FloatRegister::FromCode(code));

RValueAllocation::Double(...) or even RValueAllocation(DOUBLE_REG, ...)

I know the RValueAllocation:: is obsolete, but might make it easier to read? (If there are no extra warnings because of this). Just wondering. If you decide to change it, will you also change the others beneath?

::: js/src/vm/Stack.cpp
@@ +1179,5 @@
>       case JIT: {
>  #ifdef JS_ION
>          if (data_.ionFrames_.isOptimizedJS())
> +            return ionInlineFrames_.snapshotIterator().allocations() -
> +                ionInlineFrames_.script()->nfixed();

Since this is now multiple lines you need to add {} to the if

Comment 15

[Hannes Verschore [:h4writer]]

Comment on attachment 8370703 [details] [diff] [review]
[part 4] Merge Headers and rename Slot to RValueAllocation.

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

::: js/src/jit/Snapshots.cpp
@@ +31,5 @@
>  //   [ptr] A fixed-size pointer.
>  //   [vwu] A variable-width unsigned integer.
>  //   [vws] A variable-width signed integer.
>  //    [u8] An 8-bit unsigned integer.
> +//  [rva*] Information on how to find a js::Value in an Ion frame. The

I actually wanted you to add recover value allocations here instead of above ;)

Comment 16

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #14)
> ::: js/src/jit/Snapshots.cpp
> @@ +115,5 @@
> > +
> > +    switch (type) {
> > +      case JSVAL_TYPE_DOUBLE:
> > +        if (code < MIN_REG_FIELD_ESC)
> > +            return Double(FloatRegister::FromCode(code));
> 
> RValueAllocation::Double(...) or even RValueAllocation(DOUBLE_REG, ...)
> 
> I know the RValueAllocation:: is obsolete, but might make it easier to read?
> (If there are no extra warnings because of this). Just wondering. If you
> decide to change it, will you also change the others beneath?

If I had to do it, I would add them on others, thus I do not think a RValueAllocation will improve anything here.  I do not want to use the DOUBLE_REG value as an immediate here, as the goal was to abstract from these constructor and only give a named static constructor interface for the CodeGenerator.

What I wanted to do here, is to make a parallel with the encoding of RValueAllocation, such as we can clearly trace from where they come from (encodingAllocations), to where they are going (readAllocation)

> ::: js/src/vm/Stack.cpp
> @@ +1179,5 @@
> >       case JIT: {
> >  #ifdef JS_ION
> >          if (data_.ionFrames_.isOptimizedJS())
> > +            return ionInlineFrames_.snapshotIterator().allocations() -
> > +                ionInlineFrames_.script()->nfixed();
> 
> Since this is now multiple lines you need to add {} to the if

As I agree that this is slightly harder to read otherwise, I will do it. (not because a coding style suggest it)

Comment 17

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #15)
> ::: js/src/jit/Snapshots.cpp
> @@ +31,5 @@
> >  //   [ptr] A fixed-size pointer.
> >  //   [vwu] A variable-width unsigned integer.
> >  //   [vws] A variable-width signed integer.
> >  //    [u8] An 8-bit unsigned integer.
> > +//  [rva*] Information on how to find a js::Value in an Ion frame. The
> 
> I actually wanted you to add recover value allocations here instead of above
> ;)

Here is the new comment:

//  [rva*] list of RValueAllocation which are indicating how to find a js::Value
//         on a call/bailout. The first byte is split as thus:

Comment 18

[Nicolas B. Pierron [:nbp]]

Ok, I instrumented a build, visited the most visited websites, started a few JS games (including citadel without asm.js), and did some stats:

- Average of 61.3 Snapshots per compilations. (172744 snapshots, 2803 compilations)

   1338 [rva]     int32_t 1
   2031 [rva]     int32_t 8
   2534 [rva]     float32 (reg)
   3845 [rva]     float32 (stack)
   5225 [rva]     double (reg)
   6401 [rva]     null
   6761 [rva]     boolean (reg)
   9241 [rva]     double (stack)
  15142 [rva]     string (reg)
  25397 [rva]     boolean (stack)
  57405 [rva]     string (stack)
  80983 [rva]     constant (pool)
 130972 [rva]     object (reg)
 140443 [rva]     int32_t (reg)
 153542 [rva]     value (reg)
 212032 [rva]     int32_t 0
 235317 [rva]     value (stack)
 286437 [rva]     object (stack)
 613422 [rva]     int32_t (stack)
 838435 [rva]     undefined

Remark, As this is an x64 computer, a value can fit in one registers or in one stack slot.  This is important because we do not have enough bits of information to encode a value on one byte on 32 bits platform, even if both operands are in registers (unless we use consecutive registers).

- Stack offset with int32, objects and value's (39%).
- Undefined (29.5%), and Int32(0) (7.4%).
- Register with int32, objects and value's (15%).
- Constant pool indexes (2.8%), null (< 0.3%).
- Strings (2.6%), Booleans (1.1%).
- double & float (< 1%) (mostly caused by citadel demo).

What is interesting is that this goes against what we assumed so far for the snapshot representation, where null and undefined where 2 special values, and integers up to 31 could be encoded directly on one byte in the snapshot.

Comment 19

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f8f6414bb62
https://hg.mozilla.org/integration/mozilla-inbound/rev/f99869580d71
https://hg.mozilla.org/integration/mozilla-inbound/rev/e898d3e96948
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6b94f2a4b5

https://tbpl.mozilla.org/?tree=Try&showall=1&rev=f8aba8a8b087

Comment 20

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #18)
> Ok, I instrumented a build, visited the most visited websites, started a few
> JS games (including citadel without asm.js), and did some stats:
> 
> - Average of 61.3 Snapshots per compilations. (172744 snapshots, 2803
> compilations)

I did more stats on the logs that I made during the previous sessions, to know how many time we see the same allocations per compilations.  The reasons why I was looking at this is that we could encode any allocation on a fixed number of bits and make the snapshots both smaller and seekable(*1).

Allocations are re-used x.xx times:
  Q1:     6.23 times.
  Median: 9.91 times.
  Q3:     20.59 times.

Number of unique Allocations (*2) per compilation:
  Q1:     17 unique Allocations.
  Median: 34 unique Allocations.
  Q3:     63 unique Allocations.

Number of Allocations per compilation:
  Q1:     116 Allocations.
  Median: 467 Allocations.
  Q3:     1475 Allocations.

Note: The number of unique allocation is also likely to be higher when we are inlining functions, as we are unlikely to change the allocations of the parent function.

(*1) Having seekable snapshots has the advantage of improving the time we need to iterate over inlined frames, which is critical for walking the stack as we are currently doing a square complexity by iterating over inlined frames, by traversing snapshots linearly instead of seeking to the location of the JSFunction RValueAllocation.

(*2) The number of unique allocations is impacted by the lack of sharing of identical constants (see Bug 968296), and there is no way to check for these with the current log file.  This means that we should except a lower number of unique allocations for Q3.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f8f6414bb62
https://hg.mozilla.org/mozilla-central/rev/f99869580d71
https://hg.mozilla.org/mozilla-central/rev/e898d3e96948
https://hg.mozilla.org/mozilla-central/rev/4c6b94f2a4b5

Comment 22

[Nicolas B. Pierron [:nbp]]

Attachment: bug962555-dirty-more-than-31-float-regs.patch

Comment 23

[Nicolas B. Pierron [:nbp]]

As we are trying to figure out what is the best packing, we need to look at the distribution of size of the payloads such as stack offsets, registers and constant pool indexes. (based on the same data as comment 18)

A stack offset is a signed number (as arguments are encoded with negative values).  This implies that we need to encode the sign bit as the lowest bit to benefit from the compact buffer.  The stack is made of multiple of 4 bytes, so if by dividing by 4 we could save one bit per stack offset.  Considering this packing mechanism:

  40924  stack offsets of 3 bits ( 3.3%)
  145923 stack offsets of 4 bits (11.8%)
  294262 stack offsets of 5 bits (23.9%)
  395495 stack offsets of 6 bits (32.1%)
  251405 stack offsets of 7 bits (20.4%)
  93623  stack offsets of 8 bits ( 7.6%)
  10923  stack offsets of 9 bits
  92     stack offsets of 10 bits

  or, if represented on separated bytes:

  1128009 stack offsets are represented on 1 byte (7 bits).   (91.5%) (*)
  104638  stack offsets are represented on 2 bytes (14 bits). ( 8.5%)

A register, so far, we can go to 16 general registers, which implies that we need 4 bits to encode these.  Assuming we do not want to type the content of these registers, such as byte/32b/64b registers.

  A register is represented on 1 byte.

For float registers, so far, we can go to 32 floating point registers (5 bits) for ARM / MIPS, assuming that we type float/double separately otherwise we would go for 48 on ARM (6 bits).  On x64 we only need 16 floating point registers (4 bits) but we want to start doing auto-vectorisation at a later point we would need 2/3 extra bits to address an offset within the xmm registers.

  A float register is represented on 1 byte.

Constant pool indexes are unsigned numbers, which are corresponding to a Value index in the constant pool table. Due to Bug 968296, they are duplicated in each snapshots.  Constant pool indexes are distributed:

  2654  indexes of 1 bit.   ( 3.3%)
  2414  indexes of 2 bits.  ( 3.0%)
  4329  indexes of 3 bits.  ( 5.3%)
  7448  indexes of 4 bits.  ( 9.2%)
  10810 indexes of 5 bits.  (13.3%)
  15472 indexes of 6 bits.  (19.1%)
  18018 indexes of 7 bits.  (22.2%)
  12686 indexes of 8 bits.  (15.7%)
  5863  indexes of 9 bits.  ( 7.2%)
  1291  indexes of 10 bits. ( 1.6%)

  or, if represented on separated bytes:

  61145 indexes are represented on 1 byte (7 bits).   (75.5%)
  19840 indexes are represented on 2 bytes (14 bits). (24.5%)

(*) Compact buffer representation is done by using the sign bit to know if the number is extended to the next byte.

Comment 24

[Nicolas B. Pierron [:nbp]]

Attachment: [part 5] Move the RValueAllocation into an indexed buffer.

This patch share the RValueAllocations into a buffer and store the index of the RValueAllocation in the snapshot.

I also optimized the representation of indexes by padding serialized RValueAllocation to reduce the number of holes in the series of encoded indexes, and thus reduce the size of encoded indexes.

This patch is a needed to relax the packing that we do on RValueAllocation serialization (as we are now padding to save more space), and also a good way to make a most-of-the-time-random access on snapshots content.

Comment 25

[Nicolas B. Pierron [:nbp]]

Attachment: [part 6] Simplify encoding & decoding of RValueAllocations.

This patch takes advantage of the [part 5] which is padding on 2 bytes.  Thus we no longer need to pack RValueAllocations on only one byte, and can have a clear RValueAllocation::Mode which is almost identical(1) to the one which is encoded.

This patch basically removes the asymmetrical aspect of the encoding / decoding to make it easier to understand/extend and avoid special cases. The mode is used to find the Layout which describes how the payloads are encoded. Thus, the layout is used to dispatch to the right read/write/fprintf.

As the RValueAllocation mode fully describes its content, there is no need to keep the Location class, as we are able to maintain the same assertions by checking against the layout.

(1) It is almost identical, because when we are encoding TYPED_REG or TYPED_STACK we still want to pack the value tag in the mode.  The reason for that is that if we do not do that we will encode [mode][tag][reg/stack], which will take 3 bytes and surely waste one byte.  To save one index we still pack the tag as part of the mode, *only* when it is serialized as [mode & tag][reg/stack].

I think I should remove the all the comments which are describing the content of RValueAllocations as the layoutFromMode function is well describing that alone, in a centralized way.  What do you think?

Comment 26

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #25)
> Created attachment 8390660 [details] [diff] [review]
> [part 6] Simplify encoding & decoding of RValueAllocations.
> 

This patch also ends this clean-up bug, as it is relaxing the encoding of register to fit on one byte and not 5 bits.

Comment 27

[Hannes Verschore [:h4writer]]

Comment on attachment 8390427 [details] [diff] [review]
[part 5] Move the RValueAllocation into an indexed buffer.

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

I think the only problem I have is with the distinction of the list and map. Currently the name "snapshot" is used for the list only or for both the list and map together.
I think we should make the difference more clear.
- Snapshot: whole deal (list + map)
- SnapshotList: the list of indexes to the map (table)
- SnapshotRVAMap: what you currently call table.

(I'll highlight some of those places. But it need to get changed throughout the patch (and possibly also in places not addressed by this patch).

::: js/src/jit/Bailouts.cpp
@@ +39,5 @@
>  SnapshotIterator::SnapshotIterator(const IonBailoutIterator &iter)
> +  : SnapshotReader(iter.ionScript()->snapshots(),
> +                   iter.snapshotOffset(),
> +                   iter.ionScript()->snapshotsTableSize(),
> +                   iter.ionScript()->snapshotsSize()),

snapshotsRVAMapSize()
snapshotsListSize()

::: js/src/jit/CodeGenerator.cpp
@@ +6066,5 @@
>      // is nothing else to do after this point since the LifoAlloc memory
>      // holding the MIR graph is about to be popped and reused. In particular,
>      // every step in CodeGenerator::link must be a nop, as asserted here:
> +    JS_ASSERT(snapshots_.snapshotsSize() == 0);
> +    JS_ASSERT(snapshots_.tableSize() == 0);

listSize()
RVAMapSize()

::: js/src/jit/Ion.cpp
@@ +752,5 @@
>  
>  IonScript *
>  IonScript::New(JSContext *cx, types::RecompileInfo recompileInfo,
> +               uint32_t frameSlots, uint32_t frameSize,
> +               size_t snapshotsSize, size_t snapshotsTableSize,

snapshotsListSize, snapshotsRVAMapSize

@@ +886,5 @@
> +
> +    MOZ_ASSERT(snapshotsTableSize_);
> +    MOZ_ASSERT(writer->tableSize() == snapshotsTableSize_);
> +    memcpy((uint8_t *)this + snapshots_ + snapshotsSize_,
> +           writer->tableBuffer(), snapshotsTableSize_);

Since this is continuous, can't we copy the whole snapshot in one time?

::: js/src/jit/Snapshots.cpp
@@ +314,5 @@
>      }
>  }
>  
> +HashNumber
> +RValueAllocation::hash() const {

The name hash is really confusing here. You want to indicate this returns the content of the RVA.
I think this should be called "serialize()". That name makes more sense.

@@ +607,5 @@
> +    RValueAllocMap::AddPtr p = allocMap_.lookupForAdd(alloc);
> +    if (!p) {
> +        // Write 0x7f in all padding bytes.
> +        while (allocWriter_.length() % ALLOCATION_TABLE_ALIGNMENT)
> +            allocWriter_.writeByte(0x7f);

For me it would make much more sense that RValueAllocation::write(allocWriter) takes care to add padding. So this shouldn't be needed here.

::: js/src/jit/Snapshots.h
@@ +357,5 @@
>      }
> +
> +    HashNumber hash() const;
> +
> +    struct Hasher

s/Hasher/Serialization

@@ +378,5 @@
>      CompactBufferWriter writer_;
> +    CompactBufferWriter allocWriter_;
> +
> +    // Map RValueAllocations to an offset in the allocWriter_ buffer.  This is
> +    // useful as value allocations a repeated frequently.

s/a/are

Comment 28

[Hannes Verschore [:h4writer]]

Comment on attachment 8390660 [details] [diff] [review]
[part 6] Simplify encoding & decoding of RValueAllocations.

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

Awesome, so this allows us to already have more registers, correct?

Okay after some careful deliberation. I don't like the Mode having {type}{subtype} as integer name.
1) Poorly documented. I think most people won't understand there is a scheme to this. And they might think this is for some important reason. Which it is not.
2) When debugging and looking at the numbers in the RVAMap it might still not be easier to understand. E.g. If I would be trying to read it, I would just open an editor and look which number translates into which class
3) The switch used is now not continuous anymore. There are holes. So might get deoptimized (if it was already optimized)
So I see no real benefit in it, only confusing, just like I had before you explained and I was like "oh interesting!", sorry.

::: js/src/jit/IonFrameIterator.h
@@ +275,5 @@
>  
>      // Read an uintptr_t from the stack.
> +    bool hasStack(int32_t offset) const {
> +        return true;
> +    }

This seems a bit stupid? Why not remove "hasStack"? I think it is only used in "allocationReadable". There we can just not check StackOffset to be "hasStack" without issue...

::: js/src/jit/Snapshots.cpp
@@ +91,5 @@
>  //                  "reg" is ESC_REG_FIELD_INDEX: byte is followed by a [vws]
>  //                  stack offset containing a Value.
>  //
>  //                  Otherwise, "reg" is a register containing a Value.
>  //

This comment still needs some updating.

And also add the documentation about snapshotList and snapshotRVAMap (you may also add this in part 5 since it was introduced there).

::: js/src/jit/Snapshots.h
@@ +80,5 @@
> +    struct Layout {
> +        PayloadType type1;
> +        PayloadType type2;
> +        const char *name;
> +    };

Just a thought for later:

struct Layout {
    PackedPayloadType type1;
    PayloadType type2;
    PayloadType type3;
}
enum PackedPayLoadType {
    Packed_1bits,
    Packed_2bits,
    Packed_3bits,
    Packed_4bits
}

This would allow us to also inline other packed payloads. Not that this is needed atm. Just spewing an idea if you ever need this, it would be better to do something alike this ;)

Comment 29

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] (PTO March 3-7) from comment #27)
> @@ +886,5 @@
> > +
> > +    MOZ_ASSERT(snapshotsTableSize_);
> > +    MOZ_ASSERT(writer->tableSize() == snapshotsTableSize_);
> > +    memcpy((uint8_t *)this + snapshots_ + snapshotsSize_,
> > +           writer->tableBuffer(), snapshotsTableSize_);
> 
> Since this is continuous, can't we copy the whole snapshot in one time?

No because we have 2 different buffers, one for the snapshotList, and one for the RVAList.

> ::: js/src/jit/Snapshots.cpp
> @@ +314,5 @@
> >      }
> >  }
> >  
> > +HashNumber
> > +RValueAllocation::hash() const {
> 
> The name hash is really confusing here. You want to indicate this returns
> the content of the RVA.
> I think this should be called "serialize()". That name makes more sense.
> 
> ::: js/src/jit/Snapshots.h
> @@ +357,5 @@
> >      }
> > +
> > +    HashNumber hash() const;
> > +
> > +    struct Hasher
> 
> s/Hasher/Serialization

No, the real meaning is hashing, not serializing otherwise we would not have a for loop to rotate and xor every encoded byte from the buffer.

Serialize implies that at the end of the cable you are able unserialize, and this is not expected from a hash.

> @@ +607,5 @@
> > +    RValueAllocMap::AddPtr p = allocMap_.lookupForAdd(alloc);
> > +    if (!p) {
> > +        // Write 0x7f in all padding bytes.
> > +        while (allocWriter_.length() % ALLOCATION_TABLE_ALIGNMENT)
> > +            allocWriter_.writeByte(0x7f);
> 
> For me it would make much more sense that
> RValueAllocation::write(allocWriter) takes care to add padding. So this
> shouldn't be needed here.

I am not sure to understand why it would make more sense to do it as part of the RVA::write, but if you don't mind I will move it to the RVA::write function as part of part 6, or as a follow-up patch.

Comment 30

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] (PTO March 3-7) from comment #28)
> Awesome, so this allows us to already have more registers, correct?

Yes.
As we will have one byte to encode a register.

> Okay after some careful deliberation. I don't like the Mode having
> {type}{subtype} as integer name.

We still cannot have a linear {type} because of the packing.

> 2) When debugging and looking at the numbers in the RVAMap it might still
> not be easier to understand. E.g. If I would be trying to read it, I would
> just open an editor and look which number translates into which class

Changing the way things are numbered will not change this, you will have to open your editor if you ever have to read the raw buffer (which sounds unnecessary).

> ::: js/src/jit/IonFrameIterator.h
> @@ +275,5 @@
> >  
> >      // Read an uintptr_t from the stack.
> > +    bool hasStack(int32_t offset) const {
> > +        return true;
> > +    }
> 
> This seems a bit stupid? Why not remove "hasStack"? I think it is only used
> in "allocationReadable". There we can just not check StackOffset to be
> "hasStack" without issue...

This is only to mirror what we have with the register:

  hasRegister & readRegister.
  hasStack & readStack.

This function would be inlined and the stack offset accessor can even be removed by compiler optimizations.
I can remove it, but I prefer having a symmetry here, by principle.

> ::: js/src/jit/Snapshots.h
> @@ +80,5 @@
> > +    struct Layout {
> > +        PayloadType type1;
> > +        PayloadType type2;
> > +        const char *name;
> > +    };
> 
> Just a thought for later:
> 
> struct Layout {
>     PackedPayloadType type1;
>     PayloadType type2;
>     PayloadType type3;
> }


Ok, I'll keep that in mind for Vector registers which are coming with SIMD.

Comment 31

[Nicolas B. Pierron [:nbp]]

(part 5) https://hg.mozilla.org/integration/mozilla-inbound/rev/69bc4df5ede5
(part 6) https://hg.mozilla.org/integration/mozilla-inbound/rev/a88353443251

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/69bc4df5ede5
https://hg.mozilla.org/mozilla-central/rev/a88353443251