Store system flag in JSObject

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

11 years ago
[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.
(Assignee)

Comment 1

11 years ago
Created attachment 281520 [details] [diff] [review]
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.
Attachment #281520 - Flags: review?(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
Attachment #281520 - Flags: review?(brendan)
Attachment #281520 - Flags: review+
Attachment #281520 - Flags: approval1.9+
(Assignee)

Comment 3

11 years ago
(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.
(Assignee)

Comment 4

11 years ago
(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
(Assignee)

Comment 6

11 years ago
Created attachment 281556 [details] [diff] [review]
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.
     */
Attachment #281520 - Attachment is obsolete: true
Attachment #281556 - Flags: review?(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
Attachment #281556 - Flags: review?(brendan)
Attachment #281556 - Flags: review+
Attachment #281556 - Flags: approval1.9+
(Assignee)

Comment 8

11 years ago
(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)


(Assignee)

Comment 9

11 years ago
Created attachment 281621 [details] [diff] [review]
v1c

Patch to commit with fixed text in comments.
Attachment #281556 - Attachment is obsolete: true
Attachment #281621 - Flags: review+
Attachment #281621 - Flags: approval1.9+
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
(Assignee)

Comment 11

11 years ago
Created attachment 281704 [details] [diff] [review]
v2

I removed the static assert replacing it with a comment that refers to jsapi.h.
Attachment #281621 - Attachment is obsolete: true
Attachment #281704 - Flags: review?
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
Attachment #281704 - Flags: review?
Attachment #281704 - Flags: review+
Attachment #281704 - Flags: approval1.9+
(Assignee)

Comment 13

11 years ago
the patch, when committed, broke tinderboxes running test cases. So I took it out.
What broke, and why?

/be
(Assignee)

Comment 15

11 years ago
The patch caused start up crashes. I am checking it now.
(Assignee)

Comment 16

11 years ago
Created attachment 282289 [details] [diff] [review]
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.
Attachment #281704 - Attachment is obsolete: true
Attachment #282289 - Flags: review?(brendan)
Blocks: 392883
Comment on attachment 282289 [details] [diff] [review]
v3

D'oh!

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

Comment 18

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

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