Closed Bug 515199 Opened 10 years ago Closed 10 years ago

js_GetGCThingTraceKind must check for JSString::isStatic and return JSTRACE_STRING if so

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: regression, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

See bug 515000 comment 15.
Attached patch v1 (obsolete) — Splinter Review
The patch adds the missing check for static strings in js_GetGCThingTraceKind  and consolidate the check in few other places.
Attachment #399600 - Flags: review?(brendan)
Comment on attachment 399600 [details] [diff] [review]
v1

>-    static inline bool isUnitString(JSString *str) {
>-        return unitStringTable <= str && str < &unitStringTable[UNIT_STRING_LIMIT];
>+    static inline bool isUnitString(void *ptr) {
>+        jsuword delta = reinterpret_cast<jsuword>(ptr) -
>+                        reinterpret_cast<jsuword>(unitStringTable);
>+        if (delta >= UNIT_STRING_LIMIT * sizeof(JSString))
>+            return false;

You know I am the original byte delta unsigned less-than test-monger, over against the prettier number-line range test visible above in the - lines. Others may lament the loss of that range test, but I think your assert below justifies the hand-coded optimization:

>+
>+        /*
>+         * If ptr points inside the static array, it must point to the string.
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

This comment could fit on one line with /* and */ if you s/point to the string/be well-aligned/.

>+         * If ptr points inside the static array, it must point to the string.

Ditto comment nit.

Thanks for cleaning up. I hope none of this bites back via Shark. Setting JSVAL_STRING in the tvr is a fast minor cycle op, often it hides in memory latency in the instruction schedule. Whereas the two-table range test must cost more when marking.

Speaking of which, if we put the two tables together (adjacent) in the source file, they might be adjacent in memory and the compiler (or one of us, hand-coding) could optimize isStatic better.

Doesn't matter without some profiling evidence, just thought I'd mention it. There's no standard way to guarantee the tables are adjacent without making them the same array, of course. Which could be done, same element type (JSString)!

/be
Attachment #399600 - Flags: review?(brendan) → review+
*this* assert below:

        JS_ASSERT(delta % sizeof(JSString) == 0);

/be
Blocks: 515605
(In reply to comment #2)
> Thanks for cleaning up. I hope none of this bites back via Shark. Setting
> JSVAL_STRING in the tvr is a fast minor cycle op, often it hides in memory
> latency in the instruction schedule.

See bug 515605 about avoiding js_GetGCThingTraceKind for tvr and local roots.

> Speaking of which, if we put the two tables together (adjacent) in the source
> file, they might be adjacent in memory and the compiler (or one of us,
> hand-coding) could optimize isStatic better.

I would like to the static area for all common atoms and double constants like 0/0 and oo. With that extra checks will mounts without using one common area.
Attached patch v2Splinter Review
The new version uses better comments.
Attachment #399600 - Attachment is obsolete: true
Attachment #399704 - Flags: review+
http://hg.mozilla.org/tracemonkey/rev/f1663144bf0d
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/f1663144bf0d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Flags: wanted1.9.2+
You need to log in before you can comment on or make changes to this bug.