Closed Bug 637905 Opened 9 years ago Closed 9 years ago

Add infallibleAppend methods to Vector for use when the corresponding space has been reserve()d

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
Right now we JS_ALWAYS_TRUE them, but that's only going to trip in debug builds on OOM.  So you need methods specifically for this purpose, and the vector can internally tracked reserved size to perform the assertion in the methods.

I slightly changed some transplant methods in ways that seem correct, but Andreas should double-check those (in jsapi.cpp) just to be certain.  The rest is basically Luke-territory, or something -- the rest don't seem too domain-specific or otherwise worrisome to require more-focused knowledge of the code being changed.

This passes JS shell tests for me, will run it past a tinderbox just to be sure those are catching everything before pushing (to TM when it opens for post-4.0 stuff).
Attachment #516059 - Flags: review?(lw)
Attachment #516059 - Flags: review?(gal)
Attachment #516059 - Attachment is obsolete: true
Attachment #516059 - Flags: review?(lw)
Attachment #516059 - Flags: review?(gal)
Attachment #516211 - Flags: review?(lw)
Attachment #516211 - Flags: review?(gal)
Comment on attachment 516211 [details] [diff] [review]
Real patch, qref'd

Request which can be a follow-up: with infallibleAppend, you know that growStorageBy will succeed, so you can skip it.  This should shave off a couple always-inlined instructions; good if nothing else for binary size and i-cache locality.

>+        elts.infallibleAppend(elt); /* reserved above */

Could you remove this and all the other comments; its now implied by infallibleAppend and enforced by the mighty assert-hammer you added.

>+    /* Infallible variants usable when the corresponding space is reserved. */
>+    void infallibleAppend(const jschar c) { cb.infallibleAppend(c); }
>+    void infallibleAppend(const jschar *chars, size_t len) {
>+        cb.infallibleAppend(chars, len);
>+    }

Looks weird to me to have one all in line and the three given new lines.  Can you make the first match the others?

>+#ifdef DEBUG
>+    size_t mReserved;   /* Max elements of reserved or used space in this vector. */
>+#endif

Righteous idea.
Attachment #516211 - Flags: review?(luke) → review+
Blocks: 639583
http://hg.mozilla.org/tracemonkey/rev/30940051c457

I filed bug 639583 for the requested followup.
http://hg.mozilla.org/mozilla-central/rev/30940051c457
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.2
Attachment #516211 - Flags: review?(gal)
You need to log in before you can comment on or make changes to this bug.