Open Bug 892030 Opened 11 years ago Updated 1 year ago

Add thread safety assertions for string accesses

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect

Tracking

()

People

(Reporter: bhackett1024, Unassigned)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
Since strings can (a) be shared between compartments within a zone, and (b) mutated in place, accesses on them need to be thread safe in the presence of threads with an ExclusiveContext running in parallel with the main thread.  The attached patch adds thread safety assertions, documents how string accesses are ensured safe, and adds an exclusiveAccessLock to the runtime for use in mutating strings.  This lock will also be used for protecting other shared state in the runtime, like the atoms table.  Uses of the lock have not yet been added for string reads which may occur off thread; this will need to wait until we are closer to actually parsing JS off thread.
Attachment #773466 - Flags: review?(wmccloskey)
I forgot about strings being shared within the zone. That's really too bad.

Did you consider other options besides this one? I wonder if there's a way we can change the invariant for strings so that we don't share strings between compartments that aren't expected to be used by the same thread. I think that this is all controlled by the code in JSCompartment::wrap. If we had a predicate to determine whether a GC thing is owned by a particular thread, then we could annotate the compartment as requiring its own copies of strings. Then JSCompartment::wrap would avoid returning a string to a thread-exclusive compartment if it's not part of an arena owned by the thread; it would copy instead.

Basically, I'm wondering if we could add a "thread" field to JSCompartment and to Arena and avoid this sort of sharing.
I just realized that maybe you're worried about the use case where we want to send 10MB of JSON to a worker thread. I guess JSCompartment::wrap could allow linear, non-dependent strings to be shared between threads since they're never going to be mutated as far as I know.
Linear, non-dependent strings can be atomized, though I don't know if that would lead to races we care about.  I thought about changing this restriction, though didn't know it was all controlled by ::wrap.  My hope with this approach is that there are only a few places where we need to read the contents of strings that are not known to be atoms while having an exclusive context.  FoldConstants needs to in a few places (ConcatStrings...), and during serialization the contents of string-valued properties of objects need to be written out, but other than that I'm drawing a blank.  Most places in the engine treat strings as opaque things to pass around, and the behavior of those parts is unaffected.
> Linear, non-dependent strings can be atomized, though I don't know if that would lead to
> races we care about.

It looks to me like morphAtomizedStringIntoAtom is only called on freshly allocated strings. Are there other cases where this can happen?
(In reply to Bill McCloskey (:billm) from comment #4)
> It looks to me like morphAtomizedStringIntoAtom is only called on freshly
> allocated strings. Are there other cases where this can happen?

Oops, you're right, I didn't read AtomizeString carefully enough.

That leaves me on the fence between the two approaches.  You know how JSCompartment::wrap() is used much better than I do, so if you think isolating strings to a specific compartment would be straightforward then I can try that out.  My main concern is that we would not be able to assert this isolation in any way, since strings don't have references to their compartments.  With the approach in this bug we can assert the properties that assure thread safe accesses on the string.
Comment on attachment 773466 [details] [diff] [review]
patch

Brian and I talked about this some more and we decided that the ExclusiveContext threads could use their own zone. We'd just have to implement a "zone merging" thing to run once the thread finishes. The zone merging would just change the |zone| field of every ArenaHeader used by the zone to instead point to a main thread zone.

Using a separate zone should also make it easier for the main thread to GC while the ExclusiveContext threads are running.
Attachment #773466 - Flags: review?(wmccloskey)
Assignee: general → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: