The default bug view has changed. See this FAQ.

Thread-unsafe updates to sub-atomic rt->gc{Poke,Zeal}

RESOLVED FIXED in mozilla1.9beta2

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: brendan, Assigned: Brian Crowder)

Tracking

({fixed1.8.1.15})

Trunk
mozilla1.9beta2
fixed1.8.1.15
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.15 +
wanted1.8.1.x +
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
This came up in a conversation with Jason over IRC. The comment right above the packed gc-prefixed bools in struct JSRuntime warns against what went wrong with the patch for bug 308429. Wish I had read that when reviewing...

/be
(Reporter)

Updated

10 years ago
Blocks: 308429
(Assignee)

Comment 1

10 years ago
Created attachment 286312 [details] [diff] [review]
v1, where I should have put this in the first place.

Sorry I missed this as well.
Assignee: brendan → crowder
Status: NEW → ASSIGNED
Attachment #286312 - Flags: review?(brendan)
(Reporter)

Comment 2

10 years ago
Comment on attachment 286312 [details] [diff] [review]
v1, where I should have put this in the first place.

We should keep gc* members adjacent, if possible. But more important, do we want to interlock setting zeal with getting it? Igor, your thoughts are welcome.

/be
Attachment #286312 - Flags: review?(igor)
(Reporter)

Updated

10 years ago
Attachment #286312 - Flags: review?(brendan)
(Assignee)

Comment 3

10 years ago
Won't keeping them adjacent without padding change the shape of the runtime structure in an incompatible way?
(Reporter)

Comment 4

10 years ago
We don't preserve binary compatibility for JSRuntime or any other such private structs. The JS API does not export such structs. The few in jsapi.h are arguably frozen, but we have extended some over time. We really don't promise ABI compat, only API compat.

/be
It's worth noting that in the one case that we do have an extensible struct, there are explicit slots in which to put additional members (JSExtendedClass).

Comment 6

10 years ago
Comment on attachment 286312 [details] [diff] [review]
v1, where I should have put this in the first place.

>Index: jscntxt.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jscntxt.h,v
>retrieving revision 3.165
>diff -u -p -8 -r3.165 jscntxt.h
>--- jscntxt.h	18 Sep 2007 01:22:20 -0000	3.165
>+++ jscntxt.h	26 Oct 2007 03:49:01 -0000
>@@ -187,22 +187,17 @@ struct JSRuntime {
>     /*
>      * NB: do not pack another flag here by claiming gcPadding unless the new
>      * flag is written only by the GC thread.  Atomic updates to packed bytes
>      * are not guaranteed, so stores issued by one thread may be lost due to
>      * unsynchronized read-modify-write cycles on other threads.
>      */
>     JSPackedBool        gcPoke;
>     JSPackedBool        gcRunning;
>-#ifdef JS_GC_ZEAL
>-    uint8               gcZeal;
>-    uint8               gcPadding;
>-#else
>     uint16              gcPadding;
>-#endif
> 
>     JSGCCallback        gcCallback;
>     JSGCThingCallback   gcThingCallback;
>     void                *gcThingCallbackClosure;
>     uint32              gcMallocBytes;
>     JSGCArenaInfo       *gcUntracedArenaStackTop;
> #ifdef DEBUG
>     size_t              gcTraceLaterCount;
>@@ -416,16 +411,20 @@ struct JSRuntime {
>     jsrefcount          totalStrings;
>     jsrefcount          liveDependentStrings;
>     jsrefcount          totalDependentStrings;
>     double              lengthSum;
>     double              lengthSquaredSum;
>     double              strdepLengthSum;
>     double              strdepLengthSquaredSum;
> #endif
>+
>+#ifdef JS_GC_ZEAL
>+    uint8               gcZeal;
>+#endif
> };

It is better to have something like jsrefcount as the type for gcZeal that is guranteed to be atomic. r+ with that fixed.
(Assignee)

Comment 7

10 years ago
Created attachment 286597 [details] [diff] [review]
With Igor's and Brendan's feedback
Attachment #286312 - Attachment is obsolete: true
Attachment #286597 - Flags: review?(igor)
Attachment #286312 - Flags: review?(igor)

Updated

10 years ago
Attachment #286597 - Flags: review?(igor) → review+
(Reporter)

Updated

10 years ago
Attachment #286597 - Flags: approval1.9+
Keywords: checkin-needed
Checking in js/src/jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.169; previous revision: 3.168
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
What platforms do we support where single-byte writes are non-atomic?

Comment 10

10 years ago
(roc wants to know for bug 404870 -- see the "nsThread fix" patch there.)
on x86 single-byte writes are not atomic unless you use special atomic instructions... but a quick google search couldn't find the reference to the paper about this.
Hans Boehm seems to suggest they are atomic:
http://gcc.gnu.org/ml/java/2001-01/msg00084.html
He's an expert on this stuff, but I'd like to see a real reference one way or the other...
For what it's worth, this Microsoft article seems to agree:
http://msdn2.microsoft.com/en-us/library/bb310595.aspx
"On all modern processors, you can assume that reads and writes of naturally aligned native types are atomic."
This will be wanted on the 1.8 branch if bug 308429 is taken
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Blocks: 426628
Flags: blocking1.8.1.15? → blocking1.8.1.15+
(Assignee)

Comment 15

9 years ago
Should clear the wanted/blocking flags on these bugs, as they are replaced for the 1.8 branch by the rollup patch in bug 426628.
except that the bug description there doesn't mention this problem which means it would be very easy for QA to miss verifying parts of the fix. In the case of this bug it might not matter much, but if there were a testcase here we definitely leave bugs like this on the blocking list on purpose even when fixed by a patch in another bug.
(Assignee)

Updated

9 years ago
Keywords: fixed1.8.1.15
You need to log in before you can comment on or make changes to this bug.