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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({regression})

Trunk
regression
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

v2
7.69 KB, patch
Igor Bukanov
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
See bug 515000 comment 15.
(Assignee)

Comment 1

9 years ago
Created attachment 399600 [details] [diff] [review]
v1

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
(Assignee)

Updated

9 years ago
Blocks: 515605
(Assignee)

Comment 4

9 years ago
(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.
(Assignee)

Comment 5

9 years ago
Created attachment 399704 [details] [diff] [review]
v2

The new version uses better comments.
Attachment #399600 - Attachment is obsolete: true
Attachment #399704 - Flags: review+
(Assignee)

Comment 6

9 years ago
http://hg.mozilla.org/tracemonkey/rev/f1663144bf0d
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/f1663144bf0d
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
status1.9.2: --- → beta1-fixed
Flags: wanted1.9.2+
You need to log in before you can comment on or make changes to this bug.