4

In a recent question, we found the following piece of code:

// p, t, q and tail are Node<E> objects.
p = (p != t && t != (t = tail)) ? t : q;

Omitting the context of the question, I am interested in the following behavior:

t != (t = tail)

Considering those are the same type of objects, whatever the type. Is there any difference between this and:

t != tail

Or am I missing something crucial in the comparison mechanism ?


EDIT

If anyone wonders, this is found in the ConcurrentLinkedQueue class from java.util, line 352.

Yassine Badache
  • 1,810
  • 1
  • 19
  • 38
  • single '=' is an affectation operator. this is a twisted way to assign t to tail and then checking for its reference inequality to tail (which would always be false at this point) – spi Jan 18 '18 at 10:48
  • The difference is, in first t gets value of tail, in second not. – Vel Jan 18 '18 at 10:49
  • @spi, considering this is something we found in the `ConcurrencyLinkedQueue` class from `java.util`, this does not sound very reassuring. – Yassine Badache Jan 18 '18 at 10:50
  • 2
    @YassineBadache I hate seeing those constructs in any code. Having it in core java is indeed not much reassuring :) – spi Jan 18 '18 at 10:51
  • @spi generally these constructs are petty smart, like doing a single read for example... don't judge the code used by millions all over the world. – Eugene Jan 18 '18 at 13:20
  • My opinion does not matter much. But I bet everybody here would agree it is a pain for maintenance. You spare nanoseconds in the cost of readability which I find really awfull. – spi Jan 18 '18 at 13:24
  • @spi first of all, tagging someone via `@` is the way to get someone's attention. it's sometimes about nano-seconds, sometimes about a correct way *at all*. Things are harder then you might think: http://jeremymanson.blogspot.md/2008/12/benign-data-races-in-java.html – Eugene Jan 18 '18 at 13:26
  • @spi and if you think that getting those nano-seconds for java is not important, think about this for example: https://stackoverflow.com/a/41021171/1059372 where fields have been moved to squeeze those nanoseconds. for a library as java this *is* important – Eugene Jan 18 '18 at 13:29
  • Interesting lecture, I will give it a shot. Thank you Eugene ! – Yassine Badache Jan 18 '18 at 13:33
  • @YassineBadache np, I added that as an answer too. https://stackoverflow.com/a/48322642/1059372 – Eugene Jan 18 '18 at 13:38
  • @Eugene you're right, in a lib that matters. Maybe I underevaluated the cleverness here, but I still think anybody doing it without 1) a deep understand of what's going on and 2) a real need to this and 3) no comments to explain _why_ it's doing that way should rewrite it with a more easy to understand sequence of statements. Maintenance is really important for me, as it takes more than 80% of my time. Maybe at Oracle you are not that concerned about it, but 99% of standards programmers are. – spi Jan 18 '18 at 13:48
  • @spi that is correct, but we *are* talking in the context of Oracle lib after all; otherwise I do agree. – Eugene Jan 18 '18 at 13:49
  • I agree on the commenting part. They should have added a comment, explaining *why* this choice. It may handle some precious information about the behavior of the component. – Yassine Badache Jan 18 '18 at 14:29
  • @YassineBadache in this case `java.util.concurrent.XXX` would be 100x bigger due to the comments and let's not forget that it was *you* that looked there; no one has to document *internal details* – Eugene Jan 18 '18 at 19:39
  • @YassineBadache To be fair: The concurrent data structures are pretty well commented. Often with an elaborate `/* non-javadoc */` comment at the top and `/** JavaDocs */` even at `private` fields, and several detailed `//inline` comments. The fact that they are hard to understand nevertheless on the one hand is due to the complexity of the topic. On the other hand, ... (continued...) – Marco13 Jan 19 '18 at 19:32
  • ... the developers sometimes use certain "patterns" that lead to smaller bytecode. The latter is IMHO questionable, because maintaining such code can be a nightmare, and considering the JIT, I doubt that the benefits outweigh the hassle. But regarding this specific example: Have a look at an [earlier version of the `offer` method](https://github.com/dmlloyd/openjdk/blob/jdk6/jdk6/jdk/src/share/classes/java/util/concurrent/ConcurrentLinkedQueue.java#L217) - this was still *far* simpler and more readable. – Marco13 Jan 19 '18 at 19:33
  • 1
    For the curious: The step from the "simple" implementation to this "complex" one in the `ConcurrentLinkedQueue` seems (!) to stem from this changeset: http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/902486a8e414#l3.202 – Marco13 Jan 19 '18 at 19:40
  • @Marco13 this is an excellent find, thank you. Now I dp agree that the simpler offer is *much* more readable. I just wonder what would be the reason for such a drastic change., It has to be more than bytecode, really, this is not convincing enough. It might be just Doug Lea's preffered way of writing code – Eugene Jan 20 '18 at 01:38
  • 2
    @Eugene His style and this extreme micro-optimization has already raised [other questions](https://stackoverflow.com/q/2785964/3182664), as you know. The previous version of the code (using the `continue retry;`) is also something that I'd never accept in a code review, but this area of the codebase is *veery* special and delicate. I just think (or maybe just *hope*? - at least: I have to assume) that Doug Lea has deeply profound technical reasons to do it exactly like that. In some cases (like this one), I'd be curious to hear them, though... – Marco13 Jan 20 '18 at 01:54

3 Answers3

3
t != (t = tail)

is equivalent to

oldt = t;
t = tail;
... oldt != t...

i.e. the original value of t is compared to tail and in addition t is assigned the value of tail.

It's a short way of writing

if (t != tail) {
    t = tail;
}
Eran
  • 387,369
  • 54
  • 702
  • 768
  • 1
    I get it better. Oracle developers sometimes have some weird ways to handle things. I wonder if they had their reasons. Thank you ! – Yassine Badache Jan 18 '18 at 10:59
  • exactly wrong. the order of operations in your code is backwards – Abdul Ahad Jan 18 '18 at 12:37
  • 2
    @AbdulAhad I see what you claimed in your answer, but if you were right, the condition would always be evaluated to false (unless some other thread changed the value of t after the assignment and before the condition), which is not true. Try to run `int t=1; int tail=2;if (t != (t = tail)) System.out.println ("not equal");`. You'll see "not equal" is printed. – Eran Jan 18 '18 at 12:47
  • 1
    @Eran well it seems that this is not entirely *the same* thing... https://stackoverflow.com/a/48322642/1059372 sometimes this matters quite a lot: http://jeremymanson.blogspot.md/2008/12/benign-data-races-in-java.html. But again, I have not dived too much into the OP's questions code to be *very sure* – Eugene Jan 18 '18 at 13:36
  • Why would you check if `t != tail` before setting t to tail? Why not just assign t to tail either way? – Anthony Jan 18 '18 at 17:07
  • @Anthony usually there's some additional action you want to perform only if t != tail. – Eran Jan 18 '18 at 18:13
  • 2
    I think what you said about the "short way" is at least misleading (if not just wrong). The assignment `t=tail` **always** happens, and not only if they are not equal. The point is that the condition checks *whether* the value did change due to that assignment. This is somewhat clearer in the first part of your answer, but the `...` could be replaced by a proper `if`. EDIT: (Regarding your first comment above) : I think the whole point of this is to detect whether another thread modified the `tail`. – Marco13 Jan 19 '18 at 00:17
  • 1
    @Marco13 pretty darn happy it wasn't just me thinking this way. Since this is a concurrent data structure, someone might have already removed tail while you were looking at it. As far as I understand this is what this code does – Eugene Jan 19 '18 at 11:58
3

The first code:

t != (t = tail)

will assign tail to t then compare t with the new value

the second one will compare t to tail

ammcom
  • 992
  • 1
  • 7
  • 24
-3

Note: the other answer is what Java is actually doing

synchronized(x) {
     if(t != (t = tail));
}

is equivalent to

synchronized(x) {
    t = tail;

    if(t != t) {
        // ...
    }
}

basically, a reference to what is assigned is returned by the ( ) operator

public class Test {
    public static void main(String[] args) {
        Integer a = 1;
        Integer b = 2;

        if(a != (b = a)) {
            System.out.println("however, there is an issue with a != (a = b), Java bug");
        } else {
            System.out.println("assignment first, obvious by inspection");
        }
    }
}

However, the same code works in C. If I had to guess, this is unintentional in Java, any departure from C on something like this is folly. Oracle probably isn't looking forward to deprecating it, assuming it is a unintentional bug, which it probably is.

I'll get to it the next time I talk to them. I'm still mad at them regarding that whole Abode fiasco at the DOD related to setting my mom's homepage to ask.com in collusion with Apple Incorporated. Apple fixed iTunes in less than a week after I had to click the thing over 400 times in order to retry after re-download failures of my daughter's video library though, so the issue is limited to Oracle. It affected Microsoft as well so everyone was mad about it.

#include <iostream>

static int ref = 0;

class t {
public:
    t(int x) : x(x), r(ref) { ref++; }
    t(const t& o) : x(o.x), r(o.r) { }
    t& operator=(const t& o) { x = o.x; r = o.r; return *this; }
    bool operator!=(const t& o) const { return r != o.r; }
private:
    int x;
    int r;
};

int main() {
    t a(1);
    t b(2);

    if(a != (a = b)) {
        std::cout <<  "assignment\n";
    } else {
        std::cout <<  "no assignment\n";
    }

    return 0;
}
Abdul Ahad
  • 826
  • 8
  • 16
  • 1
    hummm actually not. The first reference to t that is used is evaluated before the assignation on the right side of the equals sign. I thought like you first, but this code proves it to be wrong: ```Object p=null, t="t", tail="tail", q="q"; p = (p != t && t != (t = tail)) ? t : q; System.out.println(p); // show "tail"``` – spi Jan 18 '18 at 12:45
  • Still no luck. Try again :) – spi Jan 18 '18 at 13:27