396758 - Store system flag in JSObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

[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.

Comment 1

[Igor Bukanov]

Attachment: v1

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 2

[Brendan Eich [:brendan]]

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

Comment 3

[Igor Bukanov]

(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.

Comment 4

[Igor Bukanov]

(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.

Comment 5

[Brendan Eich [:brendan]]

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

Comment 6

[Igor Bukanov]

Attachment: v1b

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 7

[Brendan Eich [:brendan]]

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

Comment 8

[Igor Bukanov]

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

Comment 9

[Igor Bukanov]

Attachment: v1c

Patch to commit with fixed text in comments.

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Igor Bukanov]

Attachment: v2

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

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Igor Bukanov]

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

Comment 14

[Brendan Eich [:brendan]]

What broke, and why?

/be

Comment 15

[Igor Bukanov]

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

Comment 16

[Igor Bukanov]

Attachment: v3

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 17

[Brendan Eich [:brendan]]

Comment on attachment 282289 [details] [diff] [review]
v3

D'oh!

/be

Comment 18

[Igor Bukanov]

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