With the current set of patches, the Recover{Writer,Reader} is just a dumy abstraction on top of the Snapshot{Writer,Reader}. I intent two split this in two patches, one which separate the data which are proper to the Recover / Snapshot in startRecover / startSnapshot. And as a second patch add the IonScript / compact buffer. These patches are expected to increase the size of the meta data that we are storing as we would be encoding a RecoverOffset in the snapshot. This is an acceptable temporary trade-off compared to the ultimate goal which is to reduce the number of Recover* compare to the number of snapshots.

Comment on attachment 8398293 [details] [diff] [review] [part 1] Extract the frame logic encoding/decoding from Snapshots. Review of attachment 8398293 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/Snapshots.cpp @@ +496,2 @@ > bailoutKind_ = BailoutKind((bits & SNAPSHOT_BAILOUTKIND_MASK) >> SNAPSHOT_BAILOUTKIND_SHIFT); > + offset_ = (bits & SNAPSHOT_ROFFSET_BITS) >> SNAPSHOT_ROFFSET_SHIFT; note: & …_BITS → & …_MASK (fixed locally)

Comment on attachment 8398373 [details] [diff] [review] [part 2] Add CompactBuffer to RecoverFrames. Review of attachment 8398373 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/Snapshots.cpp @@ +499,5 @@ > + IonSpew(IonSpew_Snapshots, "Read snapshot header with bailout kind %u", > + bailoutKind_); > + > +#ifdef TRACK_SNAPSHOTS > + readTrackSnapshot(); style-nit: indentation issue (fixed locally)

Delta: - Check the RecoverWriter for oom. - Follow the copy idiom in CodeGen::link.

Comment on attachment 8398293 [details] [diff] [review] [part 1] Extract the frame logic encoding/decoding from Snapshots. Review of attachment 8398293 [details] [diff] [review]: ----------------------------------------------------------------- Can you merge this and the following patch? I was completely thrown off with the "offset" to the recover instruction in the same buffer at a static location before the snapshot. I think both are part of 1 thing ;). ::: js/src/jit/Snapshots.cpp @@ +580,5 @@ > snapshot.allocRead_ = 0; > } > > SnapshotOffset > +SnapshotWriter::startSnapshot(RecoverOffset offset, BailoutKind kind) recoverOffset ::: js/src/jit/Snapshots.h @@ +394,3 @@ > BailoutKind bailoutKind_; > uint32_t allocRead_; // Number of slots that have been read. > + RecoverOffset offset_; // Offset of the recover instructions. s/offset_/recoverOffset_/ @@ +415,5 @@ > public: > SnapshotReader(const uint8_t *snapshots, uint32_t offset, > uint32_t RVATableSize, uint32_t listSize); > > + void init() { Init functions are for fallible things. Since nothing is fallible here this can go straight into the constructor. Note: I think you immediately removes this during the next patch. So don't worry about it. Though if I'm mistaken and this function is retained after these set of patches, please move to constructor. @@ +444,5 @@ > public: > RecoverReader(SnapshotReader &snapshot); > + void init(SnapshotReader &snapshot) { > + snapshot.init(); > + readFrame(snapshot); Here the same. As long as nothing is fallible, this should be in the constructor.

Comment on attachment 8398952 [details] [diff] [review] [part 2] Add CompactBuffer to RecoverFrames. Review of attachment 8398952 [details] [diff] [review]: ----------------------------------------------------------------- Nice ::: js/src/jit/Ion.cpp @@ +819,5 @@ > IonScript * > IonScript::New(JSContext *cx, types::RecompileInfo recompileInfo, > uint32_t frameSlots, uint32_t frameSize, > size_t snapshotsListSize, size_t snapshotsRVATableSize, > + size_t recoversSize, Are you planning on adding other things. If not, please move the things a bit up. So we don't have all this whitespace after recoversSize ::: js/src/jit/Snapshots.cpp @@ +550,5 @@ > return allocMap_.init(32); > } > > +RecoverReader::RecoverReader(SnapshotReader &snapshot, const uint8_t *recovers, uint32_t size) > + : reader_(recovers + snapshot.recoverOffset(), recovers + size), Since "recovers" can be nullptr. This can be potentially fault. I think you should do this after if(!recovers) @@ +677,5 @@ > uint32_t bits = > (uint32_t(resumeAfter) << RECOVER_RESUMEAFTER_SHIFT) | > (frameCount << RECOVER_FRAMECOUNT_SHIFT); > > + RecoverOffset offset = writer_.length(); recoverOffset ::: js/src/jit/Snapshots.h @@ +385,5 @@ > + return writer_.buffer(); > + } > + > + bool oom() const { > + return writer_.oom() || writer_.length() >= MAX_BUFFER_SIZE; Doesn't this also need to test snapshot_->oom() ?

(In reply to Hannes Verschore [:h4writer] from comment #7) > Can you merge this and the following patch? I was completely thrown off with > the "offset" to the recover instruction in the same buffer at a static > location before the snapshot. Ok, I'll merge the 2 and land them in one commit. > @@ +415,5 @@ > > public: > > SnapshotReader(const uint8_t *snapshots, uint32_t offset, > > uint32_t RVATableSize, uint32_t listSize); > > > > + void init() { > > Init functions are for fallible things. Since nothing is fallible here this > can go straight into the constructor. The reason is an ordering reason which disappear when we have independent buffers. This was temporally needed to work-around trackSnapshot which is computed before the insertion of the first frame. (In reply to Hannes Verschore [:h4writer] from comment #8) > ::: js/src/jit/Ion.cpp > @@ +819,5 @@ > > IonScript * > > IonScript::New(JSContext *cx, types::RecompileInfo recompileInfo, > > uint32_t frameSlots, uint32_t frameSize, > > size_t snapshotsListSize, size_t snapshotsRVATableSize, > > + size_t recoversSize, > > Are you planning on adding other things. If not, please move the things a > bit up. So we don't have all this whitespace after recoversSize No, I will. > ::: js/src/jit/Snapshots.h > @@ +385,5 @@ > > + return writer_.buffer(); > > + } > > + > > + bool oom() const { > > + return writer_.oom() || writer_.length() >= MAX_BUFFER_SIZE; > > Doesn't this also need to test snapshot_->oom() ? No, oom tests are independent, especially since we encode them separately later.