Update AbstractNodeQueue.java to avoid redundant reads of head in peekNode #15596

nitsanw · 2014-07-28T12:21:22Z

Avoid spinning on head when a peek/poll observes a null next but the queue is not empty.

lightbend-cla-validator · 2014-07-28T12:21:22Z

Hi @nitsanw,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement:

http://www.typesafe.com/contribute/cla

drewhk · 2014-07-28T12:23:37Z

akka-actor/src/main/java/akka/dispatch/AbstractNodeQueue.java

+        Node<T> next = tail.next();
+        if (next == null && get() != tail) {
+            // if tail != head this is not going to change until consumer makes progress
+            // we can avoid reading the head and just spin on next until it shows the f**k up


Please refrain from obscenity in comments (even masked)

Even if the urge is irresistable :)

irresistable will mean I cannot do as you asked.... sad and confused am I :-(
can you replace f**k with Belgium?

Joke aside, please change the comment.

akka-ci · 2014-07-28T12:26:02Z

Can one of the repo owners verify this patch?

drewhk · 2014-07-28T12:26:54Z

OK TO TEST

rkuhn · 2014-07-28T12:32:07Z

@nitsanw Have you benchmarked this change and found it to perform significantly better? It will probably help the queue, I’m wondering how that translates to the full Actor use-case.

language language....

akka-ci · 2014-07-28T13:46:33Z

Pull request validation: SUCCESS 👍
Refer to this link for build results: https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/400/

nitsanw · 2014-07-28T13:49:26Z

@rkuhn I have not benchmarked this change. I have benchmarked similar phenomena here: http://psy-lob-saw.blogspot.com/2014/06/jdk8-update-on-scalable-counters.html
Where the read impacts the mutators.

akka-ci · 2014-07-28T14:59:42Z

Pull request validation: SUCCESS 👍
Refer to this link for build results: https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/402/

nitsanw · 2014-07-28T20:41:07Z

Had a look at the Typesafe individual CLA. It involves printing and scanning and reading legal paperwork... I'm not going to find time for it. I'm currently working with Azul and if they have a CLA I can contribute through that one.
Please feel free to use the above code (have some authorised contributor copy and paste it if that works for you), with or without attribution :-)

ktoso · 2014-07-28T20:44:10Z

Hey Nitsan :-)
You don't have to do any real-mail stuffs thank goodness: If you want to you can sign it and keep for yourself, but once you do step 2) on http://www.typesafe.com/contribute/cla we trust you signed it - and that's checked and verified online (via github account and our api).

Cheers!

nitsanw · 2014-07-29T07:54:27Z

@ktoso I wish it said that in big red letters!!! :-) my eyes lose focus very rapidly when faced with red tape.
I pushed the button in good faith, CLA blindly 'signed'.

viktorklang · 2014-08-08T09:37:16Z

akka-actor/src/main/java/akka/dispatch/AbstractNodeQueue.java

        }
+        return next;
    }


Doesn't this assume that it's only ever the dequeuer that peeks?

I assume peek to be a consumer method. Is that not the usage intended? If peek is concurrent to poll that allows peek to return null when queue is not empty in both implementations as peekNode can return a node halfway through being polled.

Could you show me the state diagram for that?

:-) If we could get to a white board, sure. I'm a bit lame with drawing tools, let me try to convince you with text first.
Poll and peek both assume that a node found through tail.next() has a non-null value. This assumption is not challenged by the old or the new implementation.
Now poll() does this:
final Node next = peekNode(); // <- T0
if (next == null) return null;
else {
final T ret = next.value;
next.value = null; // <- T2
Unsafe.instance.putOrderedObject(this, tailOffset, next);
return ret;
}
While peek does this:
final Node n = peekNode(); // <- T1
return (n != null) ? n.value : null;// <- T3
At T1 n is not null, but by T3 n.value is null.

Of course! Thanks for reminding me, I knew that there was something I had not thought about.

Aside from peek() which is not used, it's only isEmpty() which calls peekNode()

peek() and peekNode() should be documented to only be called by the consumer. :)

There you go...

Just for completeness’ sake: in Akka peekNode is also called by the enqueuer, but that is fine since that one will see all the right things (it peeks into its own store buffer); third parties may erroneously report null but that is not an issue for us.

@rkuhn calling peekNode from any thread other than the consumer is not fine, as described above. Did I miss something about the producer which somehow negates this?

Yes, in general it is not fine, but I am pretty sure that Akka’s particular usage is fine. Sorry for being not 100% clear on that.

akka-ci · 2014-08-08T13:37:40Z

Pull request validation: FAILED 👎
Refer to this link for build results: https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/449/

viktorklang · 2014-08-08T13:47:30Z

akka-actor/src/main/java/akka/dispatch/AbstractNodeQueue.java

@@ -53,17 +61,19 @@ public final void addNode(final Node<T> n) {
    }

    public final boolean isEmpty() {
-        return peek() == null;
+        return Unsafe.instance.getObjectVolatile(this, tailOffset)) == get();


does not compile:

[error] /localhome/jenkinsakka/workspace/pr-validator-per-commit-jenkins/akka-actor/src/main/java/akka/dispatch/AbstractNodeQueue.java:64: ';' expected
[error] return Unsafe.instance.getObjectVolatile(this, tailOffset)) == get();
[error] ^
[error] 1 error

Should be:

return Unsafe.instance.getObjectVolatile(this, tailOffset) == get();

viktorklang · 2014-08-08T14:04:45Z

LGTM, waiting for PR validator to run

akka-ci · 2014-08-08T14:52:52Z

Pull request validation: SUCCESS 👍
Refer to this link for build results: https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/450/

@nitsanw

Motivation: There is not need todo redunant reads of head in peakNode as we can just spin on next() until it becomes visible. Modifications: Remove redundant reads of head in peakNode. This is based on @nitsanw's patch for akka. See akka/akka#15596 Result: Less volatile access.

rkuhn · 2014-08-18T17:45:01Z

Thanks a lot @nitsanw!

Update AbstractNodeQueue.java to avoid redundant reads of head in peekNode

@nitsanw

Motivation: There is not need todo redunant reads of head in peakNode as we can just spin on next() until it becomes visible. Modifications: Remove redundant reads of head in peakNode. This is based on @nitsanw's patch for akka. See akka/akka#15596 Result: Less volatile access.

@nitsanw

Motivation: There is not need todo redunant reads of head in peakNode as we can just spin on next() until it becomes visible. Modifications: Remove redundant reads of head in peakNode. This is based on @nitsanw's patch for akka. See akka/akka#15596 Result: Less volatile access.

@nitsanw

Motivation: There is not need todo redunant reads of head in peakNode as we can just spin on next() until it becomes visible. Modifications: Remove redundant reads of head in peakNode. This is based on @nitsanw's patch for akka. See akka/akka#15596 Result: Less volatile access.

nomagic · 2015-09-08T09:21:54Z

Shouldn't the comment read:
if tail != head this is not going to change until ~~consumer~~ the producer makes progress.
It's the producer that has yet to update node.next (hence the loop)
No code change, just left me confused

nitsanw · 2015-09-08T09:55:23Z

@nomagic you are correct :)

rkuhn · 2015-09-08T15:21:54Z

Indeed; do you want to submit a PR? (read: thanks for spotting this!)

nomagic · 2015-09-15T16:43:59Z

Doesn't seem worth it for a comment change.

BTW, You're probably aware of it, but just in case:

 // Extends AtomicReference for the "head" slot (which is the one that is appended to) since Unsafe
does not expose XCHG operation intrinsically

This is no longer true, was fixed in http://bugs.java.com/bugdatabase/view_bug.do?bug_id=7023898, at least on latest jdk 8 builds, x86_64 emits an XCHG (Yay!!)

nomagic · 2015-10-01T17:57:30Z

@rkuhn Yeah, my comment about Unsafe.getAndSet being intrinsified was more aimed at providing the "opportunity" to have AbstractNodeQueue decoupled from directly extending AtomicReference; I don't know if it's worth it (obviously would get a small memory gain at least), but then again admonitions about "premature optimization" probably don't apply here

rkuhn · 2015-10-01T18:17:38Z

AtomicReference has just a single field, so it should not make a difference whether we extend that or inline the field, or should it?

viktorklang · 2015-10-01T18:21:20Z

@rkuhn AFAIK, that's implementation (JVM) dependent. Any smart (read. production worthy) JVM is going to make inheritance cost 0 extra memory.

nomagic · 2015-10-01T18:36:14Z

It's hard to know frankly, but you guys are going through the trouble of accessing _tail via Unsafe (I wouldn't have just automatically assumed that's any faster than a direct volatile read/write - but I'm assuming you guys know better).
The thing I don't fully understand is exactly what has been intrinsified, i.e. is it just Unsafe.getAndSetObject() or is AtomicReference.getAndSet itself being intrinsified;I had always assumed that it was just the Unsafe methods that were intrinsified, but that bug report I cited suggests otherwise.
The motivation for my bringing your attention to this was the original code comment, that suggested:

since the JVM isn't intrinsifying (i.e. XCHG'ng) Unsafe.getAndSet, then
might as well extend AtomicReference

which makes sense.
But since 1 no longer holds, then 2 doesn't follow

rkuhn · 2015-10-01T18:42:37Z

Right, it wouldn’t be necessary anymore to inherit on JDK8, but there is also no downside to it that I can see, so no reason to change. But I may of course overlook something.

nomagic · 2015-10-01T18:43:31Z

And it should be pointed out, that if you look all through j.u.c code, AtomicReference is definitely being eschewed.
Now, that doesn't mean it should be, but I generally presume Doug Lea and co know what they're doing

nomagic · 2015-10-01T18:46:30Z

@rkuhn just to be clear, it was never necessary (AtomicReference just invokes Unsafe), it was just convenient;
i.e. it wouldn't break backwards compatibility to use Unsafe directly

@nitsanw

Motivation: There is not need todo redunant reads of head in peakNode as we can just spin on next() until it becomes visible. Modifications: Remove redundant reads of head in peakNode. This is based on @nitsanw's patch for akka. See akka/akka#15596 Result: Less volatile access.

Update AbstractNodeQueue.java

5dbdc60

Avoid spinning on head when a peek/poll observes a null next but the queue is not empty.

drewhk reviewed Jul 28, 2014
View reviewed changes

ktoso added community-contrib and removed 0 - new labels Jul 28, 2014

nitsanw added validating and removed tested labels Jul 28, 2014

Update AbstractNodeQueue.java

e8cefbf

language language....

nitsanw added validating and removed tested labels Jul 28, 2014

nitsanw removed the needs-attention label Jul 28, 2014

nitsanw added tested and removed validating labels Jul 28, 2014

viktorklang reviewed Aug 8, 2014
View reviewed changes

akka-ci added needs-attention and removed tested labels Aug 8, 2014

viktorklang reviewed Aug 8, 2014
View reviewed changes

akka-ci added tested and removed needs-attention labels Aug 8, 2014

normanmaurer mentioned this pull request Aug 18, 2014

Avoid redundant reads of head in peakNode netty/netty#2784

Closed

rkuhn added a commit that referenced this pull request Aug 18, 2014

Merge pull request #15596 from nitsanw/master

4ce7766

Update AbstractNodeQueue.java to avoid redundant reads of head in peekNode

rkuhn merged commit 4ce7766 into akka:master Aug 18, 2014

rkuhn mentioned this pull request Oct 1, 2015

=act correct two comments in AbstractNodeQueue.java #18621

Merged

Update AbstractNodeQueue.java to avoid redundant reads of head in peekNode #15596

Update AbstractNodeQueue.java to avoid redundant reads of head in peekNode #15596

Conversation

nitsanw commented Jul 28, 2014

lightbend-cla-validator commented Jul 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Jul 28, 2014

drewhk commented Jul 28, 2014

rkuhn commented Jul 28, 2014

akka-ci commented Jul 28, 2014

nitsanw commented Jul 28, 2014

akka-ci commented Jul 28, 2014

nitsanw commented Jul 28, 2014

ktoso commented Jul 28, 2014

nitsanw commented Jul 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Aug 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viktorklang commented Aug 8, 2014

akka-ci commented Aug 8, 2014

rkuhn commented Aug 18, 2014

nomagic commented Sep 8, 2015

nitsanw commented Sep 8, 2015

rkuhn commented Sep 8, 2015

nomagic commented Sep 15, 2015

nomagic commented Oct 1, 2015

rkuhn commented Oct 1, 2015

viktorklang commented Oct 1, 2015

nomagic commented Oct 1, 2015

rkuhn commented Oct 1, 2015

nomagic commented Oct 1, 2015

nomagic commented Oct 1, 2015