Closed Bug 534590 Opened 11 years ago Closed 11 years ago

eliminating GCF_LOCK flag

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

Currently GCF_LOCK is used to implement a fast locking of JS strings for the JS_LockGCThing() API. Since FF does not use this API to lock strings and since  
this significantly complicates addressing the bug 528200 I suggest to remove this flag. Instead for code simplicity locked GC things would always be put into the hashtable.
Attached patch v1 (obsolete) — Splinter Review
Besides eliminating GCF_LOCK the patch also makes JS_GCMETER defined in DEBUG builds. This borrows the idea from the bug 516832 to prevent rotting of the metering code.
Attachment #417553 - Flags: review?(brendan)
Attached patch v2 (obsolete) — Splinter Review
The patch required a trivial merge after landing of the bug 504581.
Attachment #417553 - Attachment is obsolete: true
Attachment #417744 - Flags: review?(brendan)
Attachment #417553 - Flags: review?(brendan)
Comment on attachment 417744 [details] [diff] [review]
v2

>+struct JSGCLockHashEntry: public JSDHashEntryHdr {

Nit: prevailing style puts a space before the : in class heads, as well as after. We tend to put the { on its own line when there are superclasses, too.

> js_InitGC(JSRuntime *rt, uint32 maxbytes)
> {
>     InitGCArenaLists(rt);
>     if (!JS_DHashTableInit(&rt->gcRootsHash, JS_DHashGetStubOps(), NULL,
>                            sizeof(JSGCRootHashEntry), GC_ROOTS_SIZE)) {
>         rt->gcRootsHash.ops = NULL;
>-        return JS_FALSE;
>+        return false;
>     }
>-    rt->gcLocksHash = NULL;     /* create lazily */
>+    if (!JS_DHashTableInit(&rt->gcLocksHash, JS_DHashGetStubOps(), NULL,
>+                           sizeof(JSGCLockHashEntry), GC_ROOTS_SIZE)) {
>+        rt->gcLocksHash.ops = NULL;
>+        return true;

Why return true? Do we need to JS_DHashTableFinish rt->gcRootsHash if we fail here?

Good to get rid of GCF_LOCK and all the complicated code so tiny a bit induces!

/be
Attached patch v3 (obsolete) — Splinter Review
The new version fixes the issues from the comment 3 and removes the useless flag zeroing from the finalizer code.
Attachment #417744 - Attachment is obsolete: true
Attachment #418496 - Flags: review?(brendan)
Attachment #417744 - Flags: review?(brendan)
Attached patch v4Splinter Review
The new version makes js_DumpGCStats to print symbolic names of GC arena types instead of abstract type indexes.
Attachment #418496 - Attachment is obsolete: true
Attachment #418986 - Flags: review?(brendan)
Attachment #418496 - Flags: review?(brendan)
Attachment #418986 - Attachment description: v3 → v4
Attachment #418986 - Attachment is patch: true
Attachment #418986 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 418986 [details] [diff] [review]
v4

Looks good, but I would lower-case the names: "object", "function", etc. to reduce noise and slight confusion with object types (String vs. string). Thanks,

/be
Attachment #418986 - Flags: review?(brendan) → review+
landed with low-case name in gc stats:  
https://hg.mozilla.org/tracemonkey/rev/926cb03b54b3
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/926cb03b54b3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
blocking2.0: ? → ---
You need to log in before you can comment on or make changes to this bug.