[This is a spin-off of bug 396487 comment 14] Currently GCF_SYSTEM flag of GC things is used only to mark JSObject instances. I suggest to store the flag in the object itself using a spare bit available in JSSLOT_CLASS to simplify future GC-related work.

This is a straightforward implementation of that idea to store the system flag in JSSLOT_CLASS. The patch has to add jsapi.h to prmjtime.c as that files includes jslock.h which includes jsscope.h which in turn includes jsobj.h. Since the patch added a static assert to the header, it means that JSVAL_INT must be defined prior the inclusion of jsobj.h.

Comment on attachment 281520 [details] [diff] [review] v1 It occurs to me (belatedly :-/) that we should care about atomicity and not losing bits when setting the "system" bit. This applied to the old code, where you would have to interlock with other threads that might set a gc-thing flag bit. In the new code in this patch it suffices to use JS_LOCK_OBJ(cx, obj) and UNLOCK, in js_FlagSystemObject. r/a=me with that. /be

(In reply to comment #2) > (From update of attachment 281520 [details] [diff] [review]) > It occurs to me (belatedly :-/) that we should care about atomicity and not > losing bits when setting the "system" bit. The bit can not be lost since the value of class pointer is never mutated after the creation of the object and any object once system, always system. Thus other threads simply has no API to store a value with the system bit unset given that slot updates are atomic. BTW, the current code does have a threading hazard since updates to uint8 are not atomic.

(In reply to comment #3) > BTW, the current code does have a threading hazard since updates to uint8 are > not atomic. To be precise, there are platforms where updates of uint8* is not atomic.

Sure, old code was not thread-safe as noted in comment 2. A comment about class being write-once on object creation would be nice. /be

The new version just adds the following comments to JS_FlagSystemObject: /* * We do not need to lock the object here. This method is the only API * that modifies JSSLOT_CLASS after the object is created and the slot is * initialized with object's class. Since we always set the system flag * and access to jsval is atomic, any thread that calls the method will * end up writing the same (jsval)class_pointer | 3 value into the slot. */

Comment on attachment 281556 [details] [diff] [review] v1b >+ * We do not need to lock the object here. This method is the only API >+ * that modifies JSSLOT_CLASS after the object is created and the slot is >+ * initialized with object's class. "the object's class." r+a=me with that. >Index: js/src/prmjtime.c >=================================================================== >RCS file: /cvsroot/mozilla/js/src/prmjtime.c,v >retrieving revision 3.61 >diff -U8 -p -r3.61 prmjtime.c >--- js/src/prmjtime.c 16 Jul 2007 21:29:57 -0000 3.61 >+++ js/src/prmjtime.c 19 Sep 2007 22:28:13 -0000 >@@ -45,16 +45,17 @@ > #define _REENTRANT 1 > #endif > #include <string.h> > #include <time.h> > #include "jstypes.h" > #include "jsutil.h" > > #include "jsprf.h" >+#include "jsapi.h" Necessary? grep does not show direct dependencies on jsapi.h proper. /be

(In reply to comment #7) > >Index: js/src/prmjtime.c > >+#include "jsapi.h" > > Necessary? grep does not show direct dependencies on jsapi.h proper. The patch adds to jsobj.h: JS_STATIC_ASSERT(1 == JSVAL_INT); That requires JSVAL_INT to be defined. If I remove the include, it gives the error: In file included from jsscope.h:47, from jslock.h:116, from prmjtime.c:53: jsobj.h:171: error: 'JSVAL_INT' undeclared here (not in a function)

Patch to commit with fixed text in comments.

I do not like JS_STATIC_ASSERTs in header files that drag in other .h files. Can you do that assertion elsewhere? Also it is a bit vacuous. In the current jsval scheme we really can't change anything. Any future scheme would change a lot. A static assert could be helpful, but it's not needed in every .c (.i) file that includes the .h -- put it in jsapi.c? /be

I removed the static assert replacing it with a comment that refers to jsapi.h.

Comment on attachment 281704 [details] [diff] [review] v2 Oops, no request flag set to ask me for review => missed this new patch for a while. /be

the patch, when committed, broke tinderboxes running test cases. So I took it out.

What broke, and why? /be

The patch caused start up crashes. I am checking it now.

The new version fixes JS_GetClass not to access the slots directly and use GC_AWARE_GET_CLASS instead. The next time I better make sure that when doing browser tests the code has the patch applied and compiled.

Comment on attachment 282289 [details] [diff] [review] v3 D'oh! /be

I checked in the patch from comment 16 to the CVS trunk: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1191265860&maxdate=1191265869&who=igor%mir2.org