ArC RequestContext - get rid of LazyValue for container lifecycle events

[mkouba]

• to speed up the request context activation/deactivation in majority of cases

[mkouba]

I've also added the RequestContextActivationBenchmark but the results are a bit surprising. I can see a small perf drop in the 2.12 branch, which is expected - we added some trace logging in the RequestContext. However, I can see another unexpected small perf drop in the main branch (i.e. before the commit in this pull request). Moreover, this commit does not help even though we evidently get rid of several volatile read operations.

UPDATE: I did replace the synchronized block with VarHandle.compareAndSet() and the results are much better!

In any case, I think that these changes make the code more readable. So I'd like to merge this anyway.

[Ladicek]

LGTM, I'd just call it IS_VALID instead of VALUE, but that's nitpicking.

[mkouba]

@franz1981 yet another piece of the performance puzzle :-)

[franz1981]

@mkouba

Just a thing

UPDATE: I did replace the synchronized block with VarHandle.compareAndSet() and the results are much better!

Did you tried with an volatile int field to replace the boolean one and AtomicIntegerFieldUpdater to perform the compareAndSet?
Try it if possible:

• boolean atomic ops shouldn't exists
• I don't know what var handle does here because of that(!)

[mkouba]

I did notice that AtomicBoolean is using int but I thought that it's way too much... I mean the VarHandle API is not exactly friendly.

boolean atomic ops shouldn't exists

Why is that? Lookup.findVarHandle() does not seem to forbid boolean values.

[franz1981]

Why is that? Lookup.findVarHandle() does not seem to forbid boolean values.

Because there are no atomic ops in any instruction sets involving such tiny types in any CPU archs- meaning that something is happening to make it happen...
With int updater we can see if it brings any improvement/change and what happen

[Ladicek]

FTR, my OpenJDK 11 includes atomic ops for booleans (as conversions to/from bytes, see java.lang.invoke.VarHandleBooleans and jdk.internal.misc.Unsafe) and the MethodHandles.Lookup.findVarHandle javadoc seems to agree. I'm no expert for sure, but I don't see why this shouldn't work.

[franz1981]

I don't see why this shouldn't work.

not saying it isn't work, but I don't know what it does under the hood :)
ie maybe the class field boolean used with var handle become an int (?) - it's something that worth knowing and use the right type if perf is different

[Ladicek]

The field doesn't become an int, but the CAS operation is implemented as if there was an int (with bit masking to avoid changing the nearby memory).

I guess it would be fine to make the field int directly. Currently, there's likely some padding after the boolean, so the object size should stay intact, I guess.

[franz1981]

but the CAS operation is implemented as if there was an int (with bit masking to avoid changing the nearby memory).

The field must be aligned as int to make it work, while in the field declaration (maybe) here is boolean (and JOL can show how is packed in the class layout), unless is declared volatile (and I didn't checked TBH).
If you have a reference of the assembly or what the JIT does here, would be great (on JDK source code too)

In C++ sadly makes the ops on types that doesn't support the right "grain" to become a locked one (for real), that's why I'm asking

I don't know how RequestContextState's isValid is declared here

[mkouba]

Ok, I can't say I completely understand all the comments above but let's use int. One last question - VarHandle or AtomicIntegerFieldUpdater? It seems that both use Unsafe under the hood...

[Ladicek]

My understanding has been that VarHandle is the modern replacement for Atomic*FieldUpdater, but I might easily be wrong about that :-)

[franz1981]

I've verified that the ASM produced for AtomicBoolean vs VarHande is exactly the same (for JDK 11), I'll update with more info just for the record ie -> it's fine VarHandle (yay)

And "mystery" solved too:

• var handle allow to leave boolean field at the byte granulary and just perform compare and set ops using that granularity
• AtomicInteger has been updated to use VarHandle but still at integer granulary (their asm diff slightly)

Their perf is exactly the same, because moving byte/int register(s) to memory won't make much difference (cache invalidation is still with much higher granularity anyway and hw barriers are way more costy)

hence

The field doesn't become an int, but the CAS operation is implemented as if there was an int (with bit masking to avoid changing the nearby memory).

the CAS is implemented as native byte (I was wrong, x86 cannot do it with bit-level granularity, my faulty memory, sorry) and no masking is happening if not the one implemented on ASM level eg

lock cmpxchg %r8b,0xc(%r12,%r10,8)

vs

lock cmpxchg %r8d,0xc(%r12,%r10,8)

the b vs d letter change which part of the full register to use ie 1 byte vs 4 bytes

[quarkus-bot]

Failing Jobs - Building 88e9c41

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 17 MacOS M1	Set up runner	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 18

[Ladicek]

Ah my bad, I only looked at the Java code, and there's apparently an intrinsic for the boolean (or byte) case too. Nice! However, if using AtomicBoolean ends up with the same code (the optimizer can figure out that the identity of the object is never used? Nice!), then that's probably the best.

[Sanne]

Nice!