Only fire read barriers on the main thread

RESOLVED WONTFIX

Status

()

Core
JavaScript: GC
RESOLVED WONTFIX
a year ago
9 months ago

People

(Reporter: terrence, Assigned: jonco)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox51 affected)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
This is split off from bug 844769, since we decided to land a simpler solution for that.
(Reporter)

Comment 1

a year ago
Created attachment 8790980 [details] [diff] [review]
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.
Attachment #8790980 - Flags: review+
(Reporter)

Comment 2

a year ago
Created attachment 8790982 [details] [diff] [review]
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.
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?
Does this fix any detectable race(s), or is it a tidying up that has
no effect on the race behaviour?
(Reporter)

Comment 5

a year ago
(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.
<Waldo>	sfink, terrence: I'd probably const_cast to a normal lvalue reference, then Move that
(Assignee)

Updated

a year ago
Assignee: terrence.d.cole → jcoppeard
(Assignee)

Comment 7

a year ago
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.
(Assignee)

Comment 8

a year ago
Created attachment 8803358 [details] [diff] [review]
bug1302559-remove-omt-barriers

Rebased patch.
Attachment #8790980 - Attachment is obsolete: true
Attachment #8790982 - Attachment is obsolete: true
(Assignee)

Comment 9

a year ago
Created attachment 8803359 [details] [diff] [review]
bug1302559-forward-for-rekey

Rebased patch.
(Assignee)

Comment 10

a year ago
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.
Flags: needinfo?(sphink)
(Assignee)

Comment 11

a year ago
Created attachment 8803941 [details] [diff] [review]
bug1302559-remove-omt-barriers v2

Maybe it's a simple as this...

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

Comment 12

a year ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
Flags: needinfo?(sphink)
You need to log in before you can comment on or make changes to this bug.