style: Tweak rule tree memory ordering. #15562

emilio · 2017-02-15T12:39:32Z

I've commented on the ones that I think are the most tricky. Note that this code
is stress-tested in the style tests (tests/unit/style/rule_tree/bench.rs).

This change is

highfive · 2017-02-15T12:39:35Z

Heads up! This PR modifies the following files:

@bholley: components/style/rule_tree/mod.rs

highfive · 2017-02-15T12:39:36Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio · 2017-02-15T12:40:51Z

r? @bholley

emilio · 2017-02-15T15:24:44Z

@bors-servo try

bors-servo · 2017-02-15T15:24:48Z

⌛ Trying commit e45e3bf with merge a279331...


        Auto merge of #15562 - emilio:rule-mem-order, r=<try>

style: Tweak rule tree memory ordering. I've commented on the ones that I think are the most tricky. Note that this code is stress-tested in the style tests (tests/unit/style/rule_tree/bench.rs).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15562)

bors-servo · 2017-02-15T16:02:16Z

💔 Test failed - linux-rel-wpt

emilio · 2017-02-15T17:12:54Z

Those are a few unrelated intermittents: #14338, #15444, etc., and a frameborder_a.html that seems also intermittent, let's check:

@bors-servo retry

bors-servo · 2017-02-15T17:12:58Z

⌛ Trying commit e45e3bf with merge c294699...


        Auto merge of #15562 - emilio:rule-mem-order, r=<try>

style: Tweak rule tree memory ordering. I've commented on the ones that I think are the most tricky. Note that this code is stress-tested in the style tests (tests/unit/style/rule_tree/bench.rs).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15562)

bors-servo · 2017-02-15T18:45:53Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
State: approved= try=True

bholley · 2017-02-22T03:49:17Z

components/style/rule_tree/mod.rs

-        // messing things up.
-        let ptr = self.get().next_sibling.load(Ordering::SeqCst);
+        // Note that relaxed is fine because we take care of racing if needed
+        // with the correct memory ordering at the end of our search.


Hm, what do you mean by "with the correct memory ordering" here? It seems like the reads below are relaxed as well.

Sorry for the lag here, I've reworded this comment to be clear. What I meant is that we'll eventually arrive into the CAS-loop safely in the only case the ordering may actually impact anything as far as I know.

bholley · 2017-02-22T03:49:17Z

components/style/rule_tree/mod.rs

@@ -580,13 +581,13 @@ impl StrongRuleNode {
                let existing =
                    next_sibling_ptr.compare_and_swap(ptr::null_mut(),
                                                      new_ptr,
-                                                      Ordering::SeqCst);
+                                                      Ordering::Relaxed);


Hm, at first glance it would seem that we either need to manage next_sibling_ptr (and probably first_child_ptr) with acquire/release semantics, or put a barrier between the initialization of the new RuleNode and CAS-ing it into the rule tree. Otherwise, what stops the compiler from reordering the initializing writes in RuleNode::new to occur after the CAS?

I'm not completely sure how that could happen, tbh. I can't imagine a reordering that would make sense there, given it's across a loop. Anyway, using Aquire seems safer, so let's just use that.

(Did you mean to push an update to this?)

I meant to, whoops. It's up now

bholley

Sorry for the terrible delay here. I'll try to be quick on this one going forward.

bholley · 2017-03-21T00:18:44Z

components/style/rule_tree/mod.rs

@@ -554,6 +559,7 @@ impl StrongRuleNode {
                    source: StyleSource,
                    level: CascadeLevel) -> StrongRuleNode {
        let mut last = None;
+        // TODO(emilio): We could avoid all the refcount churn here.


Given that the refcounts are atomic, I think this is worth doing. Can you file a bug against stylo-perf?

Sure, will do.

bholley · 2017-03-21T00:18:44Z

components/style/rule_tree/mod.rs

-        let mut old_head = free_list.load(Ordering::SeqCst);
+        //
+        // Note that we can use relaxed operations, we'll fail the
+        // compare_exchange_weak anyway if we fail.


This comment needs updating. Worth explaining in some detail why (Acquired, Relaxed) is the right ordering here.

bholley · 2017-03-21T00:18:44Z

components/style/rule_tree/mod.rs

+        //
+        // Note that we can use relaxed operations here since we're effectively
+        // locking the free list, and the memory ordering is already guaranteed
+        // by that locking/unlocking.


This comment needs updating as well.

bholley · 2017-03-21T00:18:44Z

components/style/rule_tree/mod.rs

+        // mutated for search/insertion. In that case, the only thing that may
+        // happen is that we find a null sibling pointer when other thread has
+        // written already to that memory location.
+        //


This still needs to be Acquire in order to synchronize properly with the Release when CAS-ing in new siblings, right?

Why? The point of this being relaxed is that even if we read when another thread has written into the field (and it's not visible), the worst case is that we'd get a null pointer when someone really wrote into it, and in that case, we'd enter the cas loop anyway, so this can be faster when there are actual siblings, which is the more common case.

I think this basically hinges on the outcome of the discussion below. If that write needs to be Release, then this read needs to be Acquire, otherwise the synchronization doesn't take effect.

bholley · 2017-03-21T00:18:44Z

components/style/rule_tree/mod.rs

@@ -580,13 +586,13 @@ impl StrongRuleNode {
                let existing =
                    next_sibling_ptr.compare_and_swap(ptr::null_mut(),
                                                      new_ptr,
-                                                      Ordering::SeqCst);
+                                                      Ordering::Acquire);


Hm, doesn't this need to be AcqRel? It needs to be Release because of the issue I pointed out last time (same reason why next_sibling() needs to use Acquire()). I think it needs to be Acquire if something reads fields on |existing|, which happens in the upgrade() call below.

Why? Upon reflection, I think even relaxed may be ok given we're looping. Aren't you mixing reads of the fields on the nodes vs reads on the atomic pointers? How's the field accesses on existing relevant to the loading order of next_sibling_ptr? (It's late and I may need re-paging all the memory orderings back in, but...)

Aren't you mixing reads of the fields on the nodes vs reads on the atomic pointers?

Unless I'm very confused here (which is a real possibility), I don't think so.

From my understanding, memory ordering isn't about ensuring any sort of consistency with regards to the actual atomic being loaded/stored (those operations are already atomic). Rather, memory ordering is about consistency in how other nearby, potentially-non-atomic operations are observed by various threads, in particularly how they may be reordered by the compiler and by the CPU. Sequential Consistency means that no other operations are observably reordered across the Sequentially-Consistent atomic operation, from the perspective of any thread. Release/Acquire means that no write operations performed by a thread manipulating field X with |Release| will be observed to be reordered across the change to X from the perspective of a thread reading field X with |Acquire|. Other threads can observe entirely different global orderings, because they're not "synchronizing" on the same memory address. [1]

So in this case, I believe we need Release when CAS-ing in the new entries, because otherwise the initialization of the new entry may not be "flushed" before the memory is accessed by other threads. And those threads need to obtain the pointers with Acquire, so that they are guaranteed to view the "flushed" writes.

If I'm wrong here, please tell me, because I would be very interested to know that. :-)

[1] http://en.cppreference.com/w/cpp/atomic/memory_order#Release-Acquire_ordering

Yeah, so there are two different things that this controls:

Visibility of non-atomic/relaxed writes, and.

Compiler instruction re-ordering.

I'm still not seeing how some re-ordering gone wrong could affect this, and wrt:

So in this case, I believe we need Release when CAS-ing in the new entries, because otherwise the initialization of the new entry may not be "flushed" before the memory is accessed by other threads.

In that case, the relaxed CAS itself wouldn't be visible either, so there's no way any other thread would be able to access that pointer, right?

Anyway, let's get down to what we agree with, and perhaps file followups as needed

In that case, the relaxed CAS itself wouldn't be visible either,
so there's no way any other thread would be able to access that pointer, right?

This is the heart of my whole concern here. There are 3 operations that happen here:
(1) The allocator selects an address in memory and returns it as a pointer.
(2) The caller writes various initialization patterns to the region in memory referenced by the pointer.
(3) The caller CASes the pointer into the rule tree, expose it to other threads.

With Relaxed semantics, what prevents another thread observing the ordering (1) (3) (2)? If nothing does, then the thread may see garbage memory.

bholley · 2017-04-02T23:46:56Z

Just a bump on this one - we're real close here so would be good to get it over the line to prevent us both from having to page everything back in all the time. :-)

emilio · 2017-04-03T08:31:58Z

Pushed the new changes as a fixup commit, in order for them to be easily reviewed. I'll squash them later.

bholley · 2017-04-03T18:06:27Z

components/style/rule_tree/mod.rs

-        // FIXME(emilio): Fiddle with memory orderings.
-        let first_child = self.first_child.load(Ordering::SeqCst);
+        // See next_sibling to see why relaxed is ok.
+        let first_child = self.first_child.load(Ordering::Relaxed);


Given that next_sibling is currently |Acquire|, this needs to be |Acquire| as well.

bholley · 2017-04-03T18:08:30Z

components/style/rule_tree/mod.rs

+                // succeeds, the writes from the last thread that wrote into
+                // next_sibling_ptr are visible in this one. If we have the
+                // guarantee that _all_ memory written before that write is
+                // visible (which I think we do), that means that their


We don't have that guarantee with Relaxed, since IIUC that's what memory ordering is all about.

bholley · 2017-04-03T18:09:03Z

components/style/rule_tree/mod.rs

+                // too, so we shouldn't need this `AcqRel` semantics.
+                //
+                // If this is true, the next_sibling() function should also be
+                // changed to `Relaxed`.


Also reference iter_children() here.

bholley · 2017-04-03T18:09:17Z

components/style/rule_tree/mod.rs

+                // guarantee that _all_ memory written before that write is
+                // visible (which I think we do), that means that their
+                // initialization of the new node is visible into our thread
+                // too, so we shouldn't need this `AcqRel` semantics.


If you do end up landing with this comment, please reference the discussion in this PR.

bholley · 2017-04-03T18:10:01Z

components/style/rule_tree/mod.rs

+    /// Returns whether this node has any child, only intended for testing
+    /// purposes.
+    pub unsafe fn has_children_for_testing(&self) -> bool {
+        !self.get().first_child.load(Ordering::Relaxed).is_null()


This should be Acquire.

This is only for testing, and there are barriers around (this is only executed in the "sequential" mode).

bholley · 2017-04-03T18:10:57Z

components/style/rule_tree/mod.rs

        };

        if !should_drop {
            return
        }

-        debug_assert_eq!(node.first_child.load(Ordering::SeqCst),
+        debug_assert_eq!(node.first_child.load(Ordering::Relaxed),


It probably doesn't matter, but this should be Acquire to be safe.

bholley · 2017-04-03T19:20:10Z

To summarize IRC discussion: All first_child and next_siblings should have:

Release stores
Acquire loads
AcqRel CASes

(excluding main-thread-only functions).

r=me with that changed and the comments fixed up. Thanks for doing this!

@bors-servo delegate+

bors-servo · 2017-04-03T19:20:11Z

✌️ @emilio can now approve this pull request


        style: Tweak rule tree memory ordering.

I've commented on the ones that I think are the most tricky. Note that this code is stress-tested in the style tests (tests/unit/style/rule_tree/bench.rs).

emilio · 2017-04-03T19:48:27Z

@bors-servo r=bholley

Sorry for being wrong on this one after all! :/

bors-servo · 2017-04-03T19:48:28Z

📌 Commit 1d94a68 has been approved by bholley

bors-servo · 2017-04-03T20:15:50Z

⌛ Testing commit 1d94a68 with merge dc8ed0d...


        Auto merge of #15562 - emilio:rule-mem-order, r=bholley

style: Tweak rule tree memory ordering. I've commented on the ones that I think are the most tricky. Note that this code is stress-tested in the style tests (tests/unit/style/rule_tree/bench.rs).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15562)

bors-servo · 2017-04-03T21:16:03Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: bholley
Pushing dc8ed0d to master...

highfive assigned cbrewster Feb 15, 2017

highfive added the S-awaiting-review label Feb 15, 2017

highfive assigned bholley and unassigned cbrewster Feb 15, 2017

highfive added the S-tests-failed label Feb 15, 2017

bholley reviewed Feb 22, 2017

View changes

bholley removed the S-awaiting-review label Feb 23, 2017

emilio force-pushed the emilio:rule-mem-order branch from e45e3bf to 56a10e9 Mar 10, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Mar 10, 2017

bholley reviewed Mar 21, 2017

View changes

emilio force-pushed the emilio:rule-mem-order branch from 56a10e9 to 6636d4b Apr 3, 2017

emilio force-pushed the emilio:rule-mem-order branch from 6636d4b to a4a6e90 Apr 3, 2017

bholley reviewed Apr 3, 2017

View changes

emilio force-pushed the emilio:rule-mem-order branch 2 times, most recently from d1d1036 to 5102d15 Apr 3, 2017

emilio force-pushed the emilio:rule-mem-order branch from 5102d15 to 1d94a68 Apr 3, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 3, 2017

highfive removed the S-awaiting-merge label Apr 3, 2017

servo / servo

Join GitHub today

style: Tweak rule tree memory ordering. #15562

style: Tweak rule tree memory ordering. #15562

Conversation

emilio commented Feb 15, 2017 • edited by larsbergstrom

highfive commented Feb 15, 2017

highfive commented Feb 15, 2017

emilio commented Feb 15, 2017

emilio commented Feb 15, 2017

bors-servo commented Feb 15, 2017

bors-servo commented Feb 15, 2017

emilio commented Feb 15, 2017

bors-servo commented Feb 15, 2017

bors-servo commented Feb 15, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley Apr 3, 2017 • edited

This comment has been minimized.

emilio Apr 3, 2017 • edited

This comment has been minimized.

This comment has been minimized.

bholley commented Apr 2, 2017

emilio commented Apr 3, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Apr 3, 2017

bors-servo commented Apr 3, 2017

emilio commented Apr 3, 2017

bors-servo commented Apr 3, 2017

bors-servo commented Apr 3, 2017

bors-servo commented Apr 3, 2017

emilio commented Feb 15, 2017 •

edited by larsbergstrom

bholley Apr 3, 2017 •

edited

emilio Apr 3, 2017 •

edited