534590 - eliminating GCF_LOCK flag

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

Attachment: v1

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.

Comment 2

[Igor Bukanov]

Attachment: v2

The patch required a trivial merge after landing of the bug 504581.

Comment 3

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

Comment 4

[Igor Bukanov]

Attachment: v3

The new version fixes the issues from the comment 3 and removes the useless flag zeroing from the finalizer code.

Comment 5

[Igor Bukanov]

Attachment: v4

The new version makes js_DumpGCStats to print symbolic names of GC arena types instead of abstract type indexes.

Comment 6

[Brendan Eich [:brendan]]

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

Comment 7

[Igor Bukanov]

landed with low-case name in gc stats:  
https://hg.mozilla.org/tracemonkey/rev/926cb03b54b3

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/926cb03b54b3