1302559 - Only fire read barriers on the main thread

Status: RESOLVED WONTFIX

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

This is split off from bug 844769, since we decided to land a simpler solution for that.

Comment 1

[Terrence Cole [:terrence]]

Attachment: bug844769-forward-for-rekey-v0.diff

This is Jon's rekey patch from bug 844769, r+'d by Steve.

We should go ahead and land this as I think it's just an improvement over the semantics that are there now.

Comment 2

[Terrence Cole [:terrence]]

Attachment: do_not_fire_read_barriers_omt-wip1.diff

This is the rebased main patch from bug 844769. It does not build on top of Jon's fixed rekeying patch. For reasons that I cannot divine, e.rekeyFront(l, Move(entry)) is triggering the [deleted] copy constructor on InitialShapeEntry. Spent too much time banging my head against this today: maybe someone else can figure out what C++ wants this time.

Comment 3

[Steve Fink [:sfink] [:s:]]

It's because by the time you get to http://dxr.mozilla.org/mozilla-central/source/js/public/HashTable.h#1856

        typename HashTableEntry<T>::NonConstT t(mozilla::Move(*p));

it is templatized on «const» T due to http://dxr.mozilla.org/mozilla-central/source/js/public/HashTable.h#326

    typedef detail::HashTable<const T, SetOps, AllocPolicy> Impl;

inside HashSet.

If you remove the const, it works. Or if you explicitly cast:

   typename Entry::NonConstT temp(const_cast<typename Entry::NonConstT&&>(*p));

which is perhaps the right thing, given all the const casting in HashSet::replaceKey?

Comment 4

[Julian Seward [:jseward]]

Does this fix any detectable race(s), or is it a tidying up that has
no effect on the race behaviour?

Comment 5

[Terrence Cole [:terrence]]

(In reply to Julian Seward [:jseward] from comment #4)
> Does this fix any detectable race(s), or is it a tidying up that has
> no effect on the race behaviour?

No, Jon's patch in bug 844769 fixed the one race we know about here. This is just so that we can assert unilaterally instead of making a thread-safe branch.

Comment 6

[Steve Fink [:sfink] [:s:]]

<Waldo>	sfink, terrence: I'd probably const_cast to a normal lvalue reference, then Move that

Comment 7

[Jon Coppeard (:jonco)]

The do_not_fire_read_barriers_omt patch causes assertion failures on try.  The read-barriered members of ObjectGroupCompartment::PlainObjectEntry are causing barriers to fire during off-main-thread parsing.  More work required.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1302559-remove-omt-barriers

Rebased patch.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: bug1302559-forward-for-rekey

Rebased patch.

Comment 10

[Jon Coppeard (:jonco)]

Steve, if you've got some sparse cycles, do you think you could take a look at this?  I think we just need to change the code relating to ObjectGroupCompartment::PlainObjectEntry in the same way as we did for the other tables that can be access off main thread.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: bug1302559-remove-omt-barriers v2

Maybe it's a simple as this...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75176ebcf78aeb762ae7cd38fdb04de6f2a0a12a

Comment 12

[Jon Coppeard (:jonco)]

Nope, lots of things (e.g. TI) end up triggering read barriers.

I'm coming to the opinion that this approach is not really an improvement.  We have to thread a context through everywhere (and we don't always have one - e.g. IonBuilder) and end up making a dynamic check on that.  Currently we have to do two checks when we fire the read barrier (on the zone's needsIncrementalBarrier flag and on the mark bits), so this might make OMT code marginally faster but I doubt it would be significant.  And I don't think it really makes us any safer.