0

In some library implementations people use to create a lot of local variables that directly relate to instance variables. I don't really get the point. Maybe someone could shine some light in my darkness.

An example is the method tryEmitNext(T t) of the EmitterProcessor in Project Reactor core library. The code is:

@Override
public EmitResult tryEmitNext(T t) {
    if (done) {
        return Sinks.EmitResult.FAIL_TERMINATED;
    }

    Objects.requireNonNull(t, "onNext");

    Queue<T> q = queue;

    if (q == null) {
        if (Operators.setOnce(S, this, Operators.emptySubscription())) {
            q = Queues.<T>get(prefetch).get();
            queue = q;
        }
        else {
            for (; ; ) {
                if (isCancelled()) {
                    return EmitResult.FAIL_CANCELLED;
                }
                q = queue;
                if (q != null) {
                    break;
                }
            }
        }
    }

    if (!q.offer(t)) {
        return subscribers == EMPTY ? EmitResult.FAIL_ZERO_SUBSCRIBER : EmitResult.FAIL_OVERFLOW;
    }
    drain();
    return EmitResult.OK;
}

My question here: Why the local q? To my understanding, everything here could be implemented, just using queue. For me it looks like, the local variable is just adding code and thus complexity without any benefit.

Note: The method Operator.setOnce(...) tries to set a subscription, and returns false in case there is already one, or if the pipeline is cancelled already.

PeMa
  • 1,559
  • 18
  • 44
  • Probablu to get a "snapshot" of current value of non thread safe values – Antoniossss Mar 29 '21 at 21:01
  • Is `queue` defined as `volatile`? – Burak Serdar Mar 29 '21 at 21:04
  • 2
    Yup. This is about concurrency. Because otherwise, the value of `queue` could change out from under you... – Louis Wasserman Mar 29 '21 at 21:05
  • @BurakSerdar queue indeed is volatile. – PeMa Mar 29 '21 at 21:28
  • Hmm, I think, I got it. This is necessary for the `for` loop that is busy waiting for the volatile `queue` to become non-null, and thus always needs to re-read the state. Thanks, for the comments. – PeMa Mar 29 '21 at 21:33
  • @LouisWasserman The value of queue is maximally set once, which is controlled by the `if` statement. The `queue` itself is concurrent and thus state changes are thread-safe. I thus this, one could directly do `queue = Queues.get(prefetch).get()` and `queue.offer()`. – PeMa Mar 29 '21 at 21:46
  • 1
    @PeMa, volatile read/writes are more expensive than normal read/writes. This pattern is usually used to increase performance a bit by getting a snapshot of the volatile variable, and operating on the non-volatile snapshot. That said, the busy-loop is simply avoidable and bad programming in my opinion, and unless there is something we don't see, the first part of that if statement has a race condition and allows setting of the `queue` by multiple threads. – Burak Serdar Mar 29 '21 at 21:47
  • @BurakSerdar Yeah, performance is probably the reason. Even though it is only relevant for the first access, where the queue actually is null. However, I don't get why there is a race condition. The `Operator.setOnce()` is thread-safe and can only be `true` once in the lifecycle of the instance. The whole code is visible here https://github.com/reactor/reactor-core/blob/master/reactor-core/src/main/java/reactor/core/publisher/EmitterProcessor.java . – PeMa Mar 29 '21 at 22:03
  • 1
    When `if q == null` runs, there is a chance that `queue` is already set by another thread and it is non-null. – Burak Serdar Mar 29 '21 at 22:05
  • @BurakSerdar Thanks for your comments. I think, I entirely got your point. Even thought, this isn't my code, it was quite helpful for understanding. – PeMa Mar 29 '21 at 22:39
  • 1
    Related, possibly duplicate: [In ArrayBlockingQueue, why copy final member field into local final variable?](https://stackoverflow.com/questions/2785964/in-arrayblockingqueue-why-copy-final-member-field-into-local-final-variable) – Mark Rotteveel Mar 30 '21 at 09:20

0 Answers0