Closed Bug 128150 Opened 22 years ago Closed 17 years ago
race condition in property cache
PROPERTY_CACHE_FILL increments cache->fills before calling PCE_STORE. There is no protection against PCE_STORE racing with PCE_LOAD on another thread. That call to PCE_LOAD could execute entirely with a single value of cache->fills and load an inconsistent property cache entry. We have seen this happen in practice several times. I'm attaching a patch which creates a second counter; one counter is incremented before PCE_STORE, the other after. PCE_LOAD checks that both counters are equal and that they have not changed during the load. A few caveats about the patch: (1) It is untested, alas. (We're not currently running with an up-to-date JS engine, but the bug is still present in tip of tree.) (2) It might not correctly handle multiprocessor situations in all cases, but it does constitute a uniprocessor fix.
Those macros predate JS_THREADSAFE, and then I got tired. My secret shame! Shaver's helping. I think we should get a fix in 0.9.9. /be
shaver, brendan, is this really an 0.9.9 stopper? If so can we get the patch tested, reviewed and it in today?
Still want this for 0.9.9, but my brain has turned to quacamole. Sleep, then comment.... /be
Can't justify holding 0.9.9 for this if it's not biting the big M (it is biting tellme.com, shaver and I are still working on the patch, it's a top priority for mozilla1.0). /be
Ok, shaver got me some MMX-based love for modern gcc/x86 platforms. Cc'ing jdunn and wtc -- can you guys help by identifying existing intrinsics or OS headers to include for atomic double-word (two pointers in a row, wrapped in a struct) loads and stores? /be
Brendan, What you want to do is usually possible in a 32-bit OS running on top of a 64-bit processor. Unfortunately, NSPR's PR_Atomic routines operate on 32-bit integers, so I am not familiar with atomic double-word loads and stores.
Testing welcome, this obviously needs #ifdef'ing. /be
brendan, is this an RC2 stopper? If it is can we get reviews and super-reviews quickly and get this into the tree Wednesday? If it's not a stopper can you remove the blocks 138000?
I'll have a better patch today, have this in the trunk as soon as reviewers and platform buddies vouch for it. /be
Your configure checks needs to check that the assembler is new enough understands these instructions, too. Since all mozilla builds build for i386 by default, most people won't use this in any event, though.
is this going to get rc3 love in the next day or two?
Whiteboard: platform issues?
js1.5 will be mozilla1.0.1, yay. /be
Moving out a bit. The js1.5 release (after an rc5 release candidate) should come off off the 1.0 branch on or before 1.0.2 freezing, I say. The trunk and branch should be in sync for js/src/js*.[chmt]* and build files. /be
Fixing TM. /be
Target Milestone: mozilla1.0.2 → mozilla1.3beta
Target Milestone: mozilla1.3beta → mozilla1.4beta
Target Milestone: mozilla1.4beta → mozilla1.5alpha
Target Milestone: mozilla1.5alpha → mozilla1.6alpha
Target Milestone: mozilla1.6alpha → mozilla1.7alpha
Hoping for some progress here soon. /be
Target Milestone: mozilla1.7alpha → mozilla1.8alpha
Target Milestone: mozilla1.8alpha1 → mozilla1.8beta2
Target Milestone: mozilla1.8beta2 → mozilla1.8beta3
Target Milestone: mozilla1.8beta3 → mozilla1.9alpha
The property cache is incorrect on MP systems, bloaty in its jsinterp.c macrology, and much less effective than polymorphic inline caches, which I'm working on in the context of bug 365851. This will regress performance for some benchmarks, but it will be interesting to see whether it affects any of the Mozilla tinderbox "T" metrics. /be
Attachment #250873 - Flags: review?(igor.bukanov)
Comment on attachment 250873 [details] [diff] [review] remove property cache I like patches dominated by munuses.
Attachment #250873 - Flags: review?(igor.bukanov) → review+
Fixed on trunk: js/src/jsapi.c rev 3.303 js/src/jscntxt.h rev 3.135 js/src/jsgc.c rev 3.198 js/src/jsinterp.c rev 3.318 js/src/jsinterp.h rev 3.52 js/src/jsobj.c rev 3.316 I'll note tinderbox Tp/Ts/Tdhtml/Txul results in a bit. /be
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Forgot to prune two sprop locals, now unused: js/src/jsobj.c rev 3.317 /be
No big bad jumps. Slight Tdhtml ding? Cc'ing bz in case he can eyeball quickly and see something serious. Again I'm working on not just restoring lost perf by eliminating the direct runtime-wide property cache -- I'm hoping to optimize calls and gets and sets quite a bit. /be
Doesn't seem to be much of an effect on Tdhtml, actually, though I'm still getting used to what numbers I can expect from this box. Sadly, the test is pretty noisy. :(
You need to log in before you can comment on or make changes to this bug.