Closed Bug 989748 Opened 8 years ago Closed 8 years ago

Abstract reads of recovery meta-data under RResumePoint

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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)
Attachment #8399102 - Flags: review?(jdemooij)
Blocks: 989759
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 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?
Attachment #8399102 - Flags: review?(jdemooij)
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} ?
Attachment #8399102 - Attachment is obsolete: true
Attachment #8400105 - Flags: review?(jdemooij)
Attachment #8400105 - Attachment is obsolete: true
Attachment #8400105 - Flags: review?(jdemooij)
Attachment #8400518 - Flags: review?(jdemooij)
(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..
Attachment #8400518 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/d3c437491728
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.