989748 - Abstract reads of recovery meta-data under RResumePoint

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

This is the next logical step after Bug 989667.  The goal is to add symmetry between the writer and the reader, and thus it makes sense to have a RResumePoint to request information about the frame.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Add RResumePoint to read recovery data.

This patch extract the RResumePoint from the RecoverReader.  The RResumePoint is now becoming the recovery-mirror of the MResumePoint.

This patch adds Recover.h, but avoids including it in any other header, only in cpp files, as this files will contained a list of recovered instructions in a near future, we do not want it to leak into the entire engine through IonFrameIterator.h.

The reason behind the Storage is that we would expect different kind of instructions to be recovered.  The storage is just a reserved space to construct one element on the stack (where the SnapshotIterator is) without having a variant of each recover instruction nor adding a fallible operation that low in the chain of functions. (that would be the worse, especially since we have NoGC iterators)

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8399102 [details] [diff] [review]
Add RResumePoint to read recovery data.

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

::: js/src/jit/Recover.cpp
@@ +82,5 @@
> +
> +RResumePoint::RResumePoint(CompactBufferReader &reader)
> +{
> +    static_assert(sizeof(Storage) <= sizeof(*this),
> +                  "Storage space is too small to decode this recover instruction.");

fixed locally: The condition happens to be true for the current structures, but it is in fact inverted.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8399102 [details] [diff] [review]
Add RResumePoint to read recovery data.

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

Looks pretty good, but would like to take another look after comments below are addressed.

::: js/src/jit/IonFrameIterator.h
@@ +296,3 @@
>      }
> +
> +    uint32_t allocations() const;

numAllocations() to make it clear it's the count. Also matches nicely with numAllocationsRead.

::: js/src/jit/Recover.h
@@ +13,5 @@
> +namespace jit {
> +
> +class CompactBufferReader;
> +
> +typedef mozilla::AlignedStorage<MAX_RINSTRUCTION_SIZE> Storage;

"Storage" is a very generic name. RInstructionStorage? RInsStorage?

::: js/src/jit/Snapshots.h
@@ +432,5 @@
>      RecoverOffset recoverOffset() const {
>          return offset_;
>      }
> +
> +    uint32_t numAllocationRead() const {

numAllocationsRead (extra s)

@@ +435,5 @@
> +
> +    uint32_t numAllocationRead() const {
> +        return allocRead_;
> +    }
> +    void resetNumAllocationRead() {

Same here.

@@ +452,5 @@
>      bool resumeAfter_;
>  
> +    // Space is reserved as part of the RecoverReader to avoid allocations of
> +    // data which is needed to decode the current instruction.
> +    mozilla::AlignedStorage<MAX_RINSTRUCTION_SIZE> rawData_;

It'd be nice if we didn't have to duplicate this definition with the one in Recover.h If we don't want to #include Recover.h here, can we move this typedef to IonTypes.h or another file?

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Add RResumePoint to read recovery data.

Delta:
 - Follow renaming nits.
 - Move RInstructionStorage to the Snapshots header, and include it in the Recover header.

Q: Should we name it RIR.{h,cpp} instead of Recover.{h,cpp} ?

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Add RResumePoint to read recovery data.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Q: Should we name it RIR.{h,cpp} instead of Recover.{h,cpp} ?

Recover.* seems a bit clearer, but I don't have a very strong opinion on this..

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2900014098ce

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G bustage in the push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d1adee6057

https://tbpl.mozilla.org/php/getParsedLog.php?id=37290188&tree=Mozilla-Inbound

Comment 9

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c437491728

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d3c437491728