Avoid unnecessary zeroing of JSGCThing

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

unspecified
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v1b
4.16 KB, patch
igor
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
The current code of js_NewGCThing in jsgc.c contains:

    /*
     * Clear thing before unlocking in case a GC run is about to scan it,
     * finding it via newborn[].
     */
    thing->next = NULL;
    thing->flagp = NULL;

Such zeroing here is unnecessary since all users of js_NewGCThing initialize the thing before doing any operation that can trigger GC. Moreover, the code does not work as a defense again bugs in the initialization code. For that it would be necessary to call memset(thing, 0, nbytes) to clear the whole thing, not just  first sizeof(JSGCThing) its bytes.

Thus I suggest to remove this zeroing.
(Assignee)

Comment 1

11 years ago
Created attachment 277712 [details] [diff] [review]
v1

js_NewObject is the only user of js_NewGCThing that relies on JSGCThing zeroing so a potential GC called from newObjectMap would see null, not junk, as JSObject.map. Thus the patch added explicit obj->map = NULL besides removing the zeroing code in js_NewGCThing.
Attachment #277712 - Flags: review?(brendan)
Attachment #277712 - Flags: approval1.9?
Comment on attachment 277712 [details] [diff] [review]
v1

>+ * can potentially cause GC. This will ensure that GC tracing never see a junk
>+ * values stored in the partially initialized thing.

s/see a/sees/

r/a=me with that.

/be
Attachment #277712 - Flags: review?(brendan)
Attachment #277712 - Flags: review+
Attachment #277712 - Flags: approval1.9?
Attachment #277712 - Flags: approval1.9+
(Assignee)

Comment 3

11 years ago
Created attachment 277750 [details] [diff] [review]
v1b

Patch to check in with nits addressed.
Attachment #277712 - Attachment is obsolete: true
Attachment #277750 - Flags: review+
Attachment #277750 - Flags: approval1.9+
(Assignee)

Comment 4

11 years ago
I checked in the patch from comment 4 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1187816103&maxdate=1187816280&who=igor%mir2.org
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.