Closed Bug 128150 Opened 22 years ago Closed 18 years ago

race condition in property cache

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: epstein, Assigned: brendan)

References

Details

(Keywords: js1.5, Whiteboard: platform issues?)

Attachments

(3 files)

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.
Attached patch Untested patchSplinter Review
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
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
No longer blocks: 122050
Keywords: mozilla0.9.9mozilla1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
Priority: P1 → P2
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
Blocks: 138000
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.
No longer blocks: 138000
Blocks: 143200
is this going to get rc3 love in the next day or two?
Whiteboard: platform issues?
No longer blocks: 143200
js1.5 will be mozilla1.0.1, yay.

/be
Keywords: mozilla1.0mozilla1.0.1
Target Milestone: mozilla1.0 → mozilla1.0.1
Blocks: 149801
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.1mozilla1.2
Target Milestone: mozilla1.0.1 → mozilla1.0.2
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)
Priority: P2 → P1
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: 18 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
QA Contact: pschwartau → general
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.  :(
Depends on: 408144
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: