Closed Bug 1213005 Opened 5 years ago Closed 5 years ago

Do not fire the read-barrier during GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We currently have a branch, but this is just because we are lazy. With just a little more care (granted, in many, many places), we can turn this into an assertion. Got everything in the shell, so let's see how a try run goes: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9d6016bc9bd
Attachment #8671542 - Flags: review?(sphink)
Comment on attachment 8671542 [details] [diff] [review]
do_not_fire_read_barriers_inside_gc-v0.diff

Ah, no. It seems our SM specific tests do not hammer on Wrappers nearly hard enough.
Attachment #8671542 - Flags: review?(sphink)
Comment on attachment 8671542 [details] [diff] [review]
do_not_fire_read_barriers_inside_gc-v0.diff

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

::: js/src/gc/Zone.cpp
@@ +311,5 @@
>  Zone::notifyObservingDebuggers()
>  {
>      for (CompartmentsInZoneIter comps(this); !comps.done(); comps.next()) {
>          JSRuntime* rt = runtimeFromAnyThread();
> +        RootedGlobalObject global(rt, comps->unsafeUnbarrieredMaybeGlobal());

I don't understand this one. Can you explain, so I don't have to go digging?

::: js/src/jsatom.cpp
@@ -199,5 @@
>          if (!entry.isPinned())
>              continue;
>  
> -        JSAtom* atom = entry.asPtr();
> -        bool tagged = entry.isPinned();

Heh. Took me a while trying to figure out what was going on here, until I noticed the check just above...

::: js/src/jsatominlines.h
@@ +30,5 @@
> +js::AtomStateEntry::asPtrUnbarriered() const
> +{
> +    MOZ_ASSERT(bits != 0);
> +    JSAtom* atom = reinterpret_cast<JSAtom*>(bits & NO_TAG_MASK);
> +    return atom;

you really want the temporary?

::: js/src/jsgc.cpp
@@ +5225,5 @@
>      for (CompartmentsIter c(rt, SkipAtoms); !c.done(); c.next()) {
>          MOZ_ASSERT(!c->gcIncomingGrayPointers);
>          for (JSCompartment::WrapperEnum e(c); !e.empty(); e.popFront()) {
>              if (e.front().key().kind != CrossCompartmentKey::StringWrapper)
> +                AssertNotOnGrayList(&e.front().value().unbarrieredGet().toObject());

This almost makes me think we ought to have an analysis -- "no read barrier calls should be dominated by calls to functions whose names start with 'Assert'". Or something.

::: js/src/jsgc.h
@@ +1276,5 @@
>  template <typename T>
>  inline void
>  CheckGCThingAfterMovingGC(const ReadBarriered<T*>& t)
>  {
> +    CheckGCThingAfterMovingGC(t.unbarrieredGet());

make that starting with 'Assert' or 'Check'

::: js/src/vm/Debugger.cpp
@@ +2530,5 @@
>      JSRuntime* rt = trc->runtime();
>      for (Debugger* dbg = rt->debuggerList.getFirst(); dbg; dbg = dbg->getNext()) {
>          WeakGlobalObjectSet& debuggees = dbg->debuggees;
>          for (WeakGlobalObjectSet::Enum e(debuggees); !e.empty(); e.popFront()) {
> +            GlobalObject* global = e.front().unbarrieredGet();

That was barriered? Ugh...
Yeah, the notifyObservingDebuggers one is really gross. The deal there is that that can be called via sweeping when we observe the debugger's target dying, or via normal mutator code when the debugger is clicked. In the non-GC case, Unbarriered access must be fine there because the rest of the refs are all also weak.
I had to back off on the initial purpose and land this with the dynamic check still in place. The problem is that HT::rekeyFront has to make a stack copy for reasons that I remember being both unavoidable and exceedingly complicated. Once we remove rekeying, we can fix this properly.
https://hg.mozilla.org/mozilla-central/rev/103c2e08b318
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.