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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

See bug 515000 comment 15.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch adds the missing check for static strings in js_GetGCThingTraceKind  and consolidate the check in few other places.

Comment 2

[Brendan Eich [: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

Comment 3

[Brendan Eich [:brendan]]

*this* assert below:

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

/be

Comment 4

[Igor Bukanov]

(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.

Comment 5

[Igor Bukanov]

Attachment: v2

The new version uses better comments.

Comment 6

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/f1663144bf0d

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/f1663144bf0d

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d44bcaf4eb9f