Closed Bug 913558 Opened 6 years ago Closed 6 years ago

GenerationalGC: why are the barriers in ObjectImpl::writeBarrierPost using runtimeFromAnyThread?

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: terrence, Unassigned)

References

Details

Attachments

(1 file)

At present, using the store buffer off-main-thread would be extremely problematic, so I would think this should be runtimeFromMainThread. This was last touched by f836042326f9: Bug 898886 - Improve threadsafe assertions when accessing runtimes and zones.
Flags: needinfo?(bhackett1024)
Attached patch beef up assertsSplinter Review
These methods can be called off thread during parsing and other future operations, so using runtimeFromMainThread() would be incorrect.  The store buffer shouldn't be used off the main thread, so the intent is that these post() barriers never trigger because nothing the non-main threads ever do will use nursery things.  All the writes done off thread are to fresh zones which don't contain nursery things, and we don't allocate from the nursery off the main thread (cx->hasNursery()).  This might need tweaking in the future since the first condition will be relaxed when we start serializing objects off thread, but for now this should hold.  The attached patch beefs up assertions in the store buffer to make sure the buffer is being used in a threadsafe way.
Attachment #801882 - Flags: review?(terrence)
Flags: needinfo?(bhackett1024)
Comment on attachment 801882 [details] [diff] [review]
beef up asserts

Review of attachment 801882 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! Thank you.

::: js/src/gc/StoreBuffer.h
@@ +443,5 @@
>  
>      /* Insert an entry into the generic buffer. */
>      template <typename T>
>      void putGeneric(const T &t) {
> +        JS_ASSERT(CurrentThreadCanAccessRuntime(runtime));

Could you move these up to MonoTypeBuffer::put and GenericBuffer::put?

Right below |JS_ASSERT(!owner->inParallelSection());|, or replacing it, if the new assertion subsumes it. You should be able to access the runtime there as |owner->runtime|.
Attachment #801882 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/0979fdfd2717
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 915167
Assignee: general → bhackett1024
Assignee: bhackett1024 → general
You need to log in before you can comment on or make changes to this bug.