On Wed, 5 Jan 2022 17:23:40 GMT, Paul Sandoz <psan...@openjdk.org> wrote:
>> This patch fixes a performance issue when dereferencing memory segments >> backed by different kinds of scopes. When looking at inline traces, I found >> that one of our benchmark, namely `LoopOverPollutedSegment` was already >> hitting the ceiling of the bimorphic inline cache, specifically when >> checking liveness of the segment scope in the memory access hotpath >> (`ResourceScopeImpl::checkValidState`). The benchmark only used segments >> backed by confined and global scope. I then added (in the initialization >> "polluting" loop) segments backed by a shared scope, and then the benchmark >> numbers started to look as follows: >> >> >> Benchmark Mode Cnt Score >> Error Units >> LoopOverPollutedSegments.heap_segment_floats_VH avgt 30 7.004 ? >> 0.089 ms/op >> LoopOverPollutedSegments.heap_segment_floats_instance avgt 30 7.159 ? >> 0.016 ms/op >> LoopOverPollutedSegments.heap_segment_ints_VH avgt 30 7.017 ? >> 0.110 ms/op >> LoopOverPollutedSegments.heap_segment_ints_instance avgt 30 7.175 ? >> 0.048 ms/op >> LoopOverPollutedSegments.heap_unsafe avgt 30 0.243 ? >> 0.004 ms/op >> LoopOverPollutedSegments.native_segment_VH avgt 30 7.366 ? >> 0.036 ms/op >> LoopOverPollutedSegments.native_segment_instance avgt 30 7.305 ? >> 0.098 ms/op >> LoopOverPollutedSegments.native_unsafe avgt 30 0.238 ? >> 0.002 ms/op >> >> >> That is, since now we have *three* different kinds of scopes (confined, >> shared and global), the call to the liveness check can no longer be inlined. >> One solution could be, as we do for the *base* accessor, to add a scope >> accessor to all memory segment implementation classes. But doing so only >> works ok for heap segments (for which the scope accessor just returns the >> global scope constants). For native segments, we're still megamorphic (as a >> native segment can be backed by all kinds of scopes). >> >> In the end, it turned out to be much simpler to just make the liveness check >> monomorphic, since there's so much sharing between the code paths already. >> With that change, numbers of the tweaked benchmark go back to normal: >> >> >> Benchmark Mode Cnt Score >> Error Units >> LoopOverPollutedSegments.heap_segment_floats_VH avgt 30 0.241 ? >> 0.003 ms/op >> LoopOverPollutedSegments.heap_segment_floats_instance avgt 30 0.244 ? >> 0.003 ms/op >> LoopOverPollutedSegments.heap_segment_ints_VH avgt 30 0.242 ? >> 0.003 ms/op >> LoopOverPollutedSegments.heap_segment_ints_instance avgt 30 0.248 ? >> 0.001 ms/op >> LoopOverPollutedSegments.heap_unsafe avgt 30 0.247 ? >> 0.013 ms/op >> LoopOverPollutedSegments.native_segment_VH avgt 30 0.245 ? >> 0.004 ms/op >> LoopOverPollutedSegments.native_segment_instance avgt 30 0.245 ? >> 0.001 ms/op >> LoopOverPollutedSegments.native_unsafe avgt 30 0.247 ? >> 0.005 ms/op >> >> >> Note that this patch tidies up a bit the usage of `checkValidState` vs. >> `checkValidStateSlow`. The former should only really be used in the hot >> path, while the latter is a more general routine which should be used in >> non-performance critical code. Making `checkValidState` monomorphic caused >> the `ScopeAccessError` to be generated in more places, so I needed to either >> update the usage to use the safer `checkValidStateSlow` (where possible) or, >> (in `Buffer` and `ConfinedScope`) just add extra wrapping. > > src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceScopeImpl.java > line 190: > >> 188: @ForceInline >> 189: public final void checkValidState() { >> 190: if (owner != null && owner != Thread.currentThread()) { > > For consistency we could change code `checkValidStateSlow` to refer directly > to `owner`. > > It would be satisfying, but I don't know if it's possible, to compose > `checkValidStateSlow` from `checkValidState` e.g. > > public final checkValidStateSlow() { > checkValidState(); > if (!isAlive() { ... } > } I'll change use of `owner`. It's not really possible to write checkValidStateSlow in terms of checkValidState, because the latter does a plain read of the state, whereas the former does a volatile read. Reusing one from the other would result in two reads (a plain and a volatile). ------------- PR: https://git.openjdk.java.net/jdk18/pull/82