Last Comment Bug 401188 - Thread-unsafe updates to sub-atomic rt->gc{Poke,Zeal}
: Thread-unsafe updates to sub-atomic rt->gc{Poke,Zeal}
Status: RESOLVED FIXED
: fixed1.8.1.15
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9beta2
Assigned To: Brian Crowder
:
Mentors:
Depends on:
Blocks: 308429 426628
  Show dependency treegraph
 
Reported: 2007-10-25 19:33 PDT by Brendan Eich [:brendan]
Modified: 2008-06-04 11:54 PDT (History)
11 users (show)
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
bob: in‑testsuite-
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1, where I should have put this in the first place. (1.78 KB, patch)
2007-10-26 10:33 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
With Igor's and Brendan's feedback (1.11 KB, patch)
2007-10-29 13:50 PDT, Brian Crowder
igor: review+
brendan: approval1.9+
Details | Diff | Splinter Review

Description Brendan Eich [:brendan] 2007-10-25 19:33:05 PDT
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
Comment 1 Brian Crowder 2007-10-26 10:33:04 PDT
Created attachment 286312 [details] [diff] [review]
v1, where I should have put this in the first place.

Sorry I missed this as well.
Comment 2 Brendan Eich [:brendan] 2007-10-26 15:56:39 PDT
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
Comment 3 Brian Crowder 2007-10-26 16:19:07 PDT
Won't keeping them adjacent without padding change the shape of the runtime structure in an incompatible way?
Comment 4 Brendan Eich [:brendan] 2007-10-26 16:35:42 PDT
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
Comment 5 Blake Kaplan (:mrbkap) 2007-10-26 16:37:30 PDT
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 Igor Bukanov 2007-10-26 17:25:09 PDT
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.
Comment 7 Brian Crowder 2007-10-29 13:50:42 PDT
Created attachment 286597 [details] [diff] [review]
With Igor's and Brendan's feedback
Comment 8 Reed Loden [:reed] (use needinfo?) 2007-11-13 02:44:25 PST
Checking in js/src/jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.169; previous revision: 3.168
done
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-11-21 19:34:56 PST
What platforms do we support where single-byte writes are non-atomic?
Comment 10 Jesse Ruderman 2007-11-21 19:37:37 PST
(roc wants to know for bug 404870 -- see the "nsThread fix" patch there.)
Comment 11 Benjamin Smedberg [:bsmedberg] 2007-11-22 05:52:43 PST
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.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-11-22 11:46:22 PST
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...
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-11-22 11:53:55 PST
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."
Comment 14 Daniel Veditz [:dveditz] 2008-04-02 15:03:23 PDT
This will be wanted on the 1.8 branch if bug 308429 is taken
Comment 15 Brian Crowder 2008-06-03 08:30:36 PDT
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.
Comment 16 Daniel Veditz [:dveditz] 2008-06-03 10:29:45 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.