Do not lock on reads of XPackLicenseState

[Tim-Brooks]

XPackLicenseState reads to necessary to validate a number of cluster
operations. This reads occasionally occur on transport threads which
should not be blocked. Currently we sychronize when reading. However,
this is unecessary as only a single piece of state is updateable. This
commit makes this state volatile and removes the locking.

[elasticmachine]

Pinging @elastic/es-security (:Security/License)

[jasontedor]

I left a question.

[jaymode]

I would like to understand whether this has been an issue in practice? Read write locks can be slow and much slower than synchronized depending on the workload. StampedLock with optimistic concurrency might be a better choice but more complex

[Tim-Brooks]

I would like to understand whether this has been an issue in practice?

XPackLicenseState synchronized blocks show up on JFR recordings as one of the highest areas of contention with many concurrent clients. It's not huge, but http and transport worker threads end up blocking each other which is not ideal. Particularly very little about this really needs to be synchronized.

Anyway, I am fine with whatever approach that avoids blocking on reads. Read write locks seem fine to me, since the write will almost never happens. Volatile read seems fine to me as it is definitely the most performant, but I understand @jasontedor concern that it may not be immediately obvious to many developers that you need to copy locally before reading multiple times.

StampedLock seems not ideal to me because we are only saving some overhead vs. read write lock. And it does not seem simpler than volatile read approach which is where we should go if we are concerned about acquire op performance.

[jasontedor]

Read write locks can be slow and much slower than synchronized depending on the workload.

This is true, but I wouldn’t expect this to be one since writes are exceptionally rare.

[jasontedor]

LGTM.

[jaymode]

I ran a JMH benchmark for this. With N threads reading a value concurrently for a ReadWriteLock, StampedLock using optimistic concurrency, synchronized variable access, and volatile reads. On my laptop with 10 warmup iterations and 10 iterations:

2 threads:

Benchmark                     Mode  Cnt          Score          Error  Units
ReadBenchmark.ReadWriteLock  thrpt   10    8172004.518 ±   548733.853  ops/s
ReadBenchmark.StampedLock    thrpt   10  601609704.280 ± 11975531.362  ops/s
ReadBenchmark.Synchronized   thrpt   10   28712159.057 ±  2126405.036  ops/s
ReadBenchmark.Volatile       thrpt   10  825214200.299 ± 25887669.478  ops/s

4 threads:

Benchmark                     Mode  Cnt           Score          Error  Units
ReadBenchmark.ReadWriteLock  thrpt   10     5802215.036 ±   577016.754  ops/s
ReadBenchmark.StampedLock    thrpt   10  1112952765.274 ± 90027447.946  ops/s
ReadBenchmark.Synchronized   thrpt   10    16757713.231 ±   424539.055  ops/s
ReadBenchmark.Volatile       thrpt   10  1501490557.569 ± 71904534.874  ops/s

10 threads:

Benchmark                     Mode  Cnt           Score           Error  Units
ReadBenchmark.ReadWriteLock  thrpt   10     4710684.063 ±    271795.958  ops/s
ReadBenchmark.StampedLock    thrpt   10  1619121029.519 ±  99809765.720  ops/s
ReadBenchmark.Synchronized   thrpt   10    14521516.416 ±    243941.973  ops/s
ReadBenchmark.Volatile       thrpt   10  2367239808.165 ± 184625480.760  ops/s

20 threads:

Benchmark                     Mode  Cnt           Score           Error  Units
ReadBenchmark.ReadWriteLock  thrpt   10     4528401.619 ±    476427.008  ops/s
ReadBenchmark.StampedLock    thrpt   10  1534485125.568 ±  91383500.473  ops/s
ReadBenchmark.Synchronized   thrpt   10    16371323.837 ±    382284.763  ops/s
ReadBenchmark.Volatile       thrpt   10  2277131203.968 ± 114318253.257  ops/s

Given this, I do not see how this change will improve the performance of indexing.

[jaymode]

One way to implement StampedLock could look something like:

    private final StampedLock stampedLock = new StampedLock();
    private final ReleasableLock readLock = new ReleasableLock(stampedLock.asReadLock());

    public boolean checkOptimistically(Predicate<Status> statusPredicate) {
        final long stamp = stampedLock.tryOptimisticRead();
        final boolean status = statusPredicate.test(counter);
        if (rwlock.validate(stamp) == false) {
            try (ReleasableLock lock = readLock.acquire()) {
                return statusPredicate.test(state);
            }
        }
        return status;
    }

Each method could call that method with a predicate to test the status object.

[Tim-Brooks]

I had not seen that the StampedLock offered a read and write mode. I thought you had to always use the permits. I changed to the StampedLock since it is compatible with what I was trying to do.

[jaymode]

I thought you had to always use the permits.

You do not have to, but not doing so degrades performance to the level of a ReentrantReadWriteLock.

I think if we're trying to avoid blocking transport threads volatile would be the best choice with a pattern such as:

    boolean checkAgainstStatus(Predicate<Status> statusPredicate) {
        return statusPredicate.test(status);
    }

This along with comments about needing to use the same status in the check should be enough or at least I think.

[jasontedor]

Looks good, thanks for the iterations. Thanks @jaymode for the convincing benchmarks that got us here.

[jaymode]

LGTM. Thanks for letting me chime in