On Wed, 23 Nov 2022 19:16:23 GMT, Paul Sandoz <psan...@openjdk.org> wrote:
> Looks good (just some last minor comments). I did not focus on the tests, nor > too closely at all of the HotSpot changes. > > Something for future investigation perhaps (if not already thought about): > consider using a persistent map, a Hash Array Mapped Trie (HAMT), for storing > scoped keys and values, which could potentially remove the need for the cache > of replace the cache when many values are in scope. The HAMT's structural > sharing properties, wide-branching factor, and `Integer.bitCount` being > intrinsic all make for an efficient implementation. I've certainly considered a HAMT. (I've considered everything! It's been a long journey.) The trouble is that using one penalizes binding operations because a persistent HAMT requires path copying for insertions. This won't matter if your bind operation is rare, such as at the start of a large block of code. However, it might not be. Consider a common case such as a parallel stream using fork/join. void parallelRunWithCredentials(List<?> aList, Runnable aRunnable) { var permissions = ScopedValue.where(PERMISSION, getCredentials()); aList.parallelStream().forEach(() -> permissions.run(aRunnable)); } Because the binding operation `permissions.run()` is invoked for every element on the list, we must make the binding operation as fast as it can possibly be. In addition, the cache is _extremely_ fast. Typically it's four instructions, and C2 almost always hoists values, once looked up in cache, into registers. A HAMT lookup, while fast and (mostly) O(1), is more complex. ------------- PR: https://git.openjdk.org/jdk/pull/10952