Closed
Bug 128150
Opened 22 years ago
Closed 17 years ago
race condition in property cache
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: epstein, Assigned: brendan)
References
Details
(Keywords: js1.5, Whiteboard: platform issues?)
Attachments
(3 files)
2.96 KB,
patch
|
Details | Diff | Splinter Review | |
1.71 KB,
patch
|
Details | Diff | Splinter Review | |
17.96 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•22 years ago
|
||
Assignee | ||
Comment 2•22 years ago
|
||
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
Blocks: 122050
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: js1.5,
mozilla0.9.9
Priority: -- → P1
Target Milestone: --- → mozilla0.9.9
Comment 3•22 years ago
|
||
shaver, brendan, is this really an 0.9.9 stopper? If so can we get the patch tested, reviewed and it in today?
Assignee | ||
Comment 4•22 years ago
|
||
Still want this for 0.9.9, but my brain has turned to quacamole. Sleep, then comment.... /be
Assignee | ||
Comment 5•22 years ago
|
||
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
No longer blocks: 122050
Keywords: mozilla0.9.9 → mozilla1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
Assignee | ||
Updated•22 years ago
|
Priority: P1 → P2
Assignee | ||
Comment 6•22 years ago
|
||
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
Comment 7•22 years ago
|
||
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.
Assignee | ||
Comment 8•22 years ago
|
||
Testing welcome, this obviously needs #ifdef'ing. /be
Comment 9•22 years ago
|
||
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?
Assignee | ||
Comment 10•22 years ago
|
||
I'll have a better patch today, have this in the trunk as soon as reviewers and platform buddies vouch for it. /be
Comment 11•22 years ago
|
||
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.
Comment 12•22 years ago
|
||
is this going to get rc3 love in the next day or two?
Whiteboard: platform issues?
Assignee | ||
Comment 13•22 years ago
|
||
js1.5 will be mozilla1.0.1, yay. /be
Keywords: mozilla1.0 → mozilla1.0.1
Target Milestone: mozilla1.0 → mozilla1.0.1
Assignee | ||
Comment 14•22 years ago
|
||
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
Keywords: mozilla1.0.1 → mozilla1.2
Target Milestone: mozilla1.0.1 → mozilla1.0.2
Assignee | ||
Updated•21 years ago
|
Target Milestone: mozilla1.3beta → mozilla1.4beta
Assignee | ||
Updated•21 years ago
|
Target Milestone: mozilla1.4beta → mozilla1.5alpha
Assignee | ||
Updated•21 years ago
|
Target Milestone: mozilla1.5alpha → mozilla1.6alpha
Assignee | ||
Updated•20 years ago
|
Target Milestone: mozilla1.6alpha → mozilla1.7alpha
Assignee | ||
Comment 16•20 years ago
|
||
Hoping for some progress here soon. /be
Target Milestone: mozilla1.7alpha → mozilla1.8alpha
Assignee | ||
Updated•19 years ago
|
Target Milestone: mozilla1.8alpha1 → mozilla1.8beta2
Assignee | ||
Updated•19 years ago
|
Target Milestone: mozilla1.8beta2 → mozilla1.8beta3
Assignee | ||
Updated•19 years ago
|
Target Milestone: mozilla1.8beta3 → mozilla1.9alpha
Assignee | ||
Comment 17•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Priority: P2 → P1
Comment 18•17 years ago
|
||
Comment on attachment 250873 [details] [diff] [review] remove property cache I like patches dominated by munuses.
Attachment #250873 -
Flags: review?(igor.bukanov) → review+
Assignee | ||
Comment 19•17 years ago
|
||
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
Assignee | ||
Comment 20•17 years ago
|
||
Forgot to prune two sprop locals, now unused: js/src/jsobj.c rev 3.317 /be
Assignee | ||
Comment 21•17 years ago
|
||
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
Updated•17 years ago
|
QA Contact: pschwartau → general
![]() |
||
Comment 22•17 years ago
|
||
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.
Description
•