On Fri, 8 Sep 2023 16:57:14 GMT, Mandy Chung <mch...@openjdk.org> wrote:
> Typically it will find the caller class at the second stack frame from the > frame of executing `StackWalker::getCallerClass`. The initial size of the > buffer can be changed from 8 to 4 (the top 2 elements are reserved for > implementation use). If it fetches another batch, `getCallerClass` may be > invoked via core reflection, so subsequent batches can be increased to a > larger size. This PR also moves the benchmark for `getCallerClass` in its > own file because it does not need to test with different depth and can be > separated from StackWalkBench. In general looks good. But once you on this, I suggest to add the following additional optimizations: - `FrameBuffer.START_POS` is `2` but as far as I can see, currently only the 0th element is reserved for passing a "magic" object passed between the JVM and Java code. So this should be set to `1`. - `MIN_BATCH_SIZE` should be defined as in terms of `FrameBuffer.START_POS` (i.e. `FrameBuffer.START_POS + 1`) - `StackFrameTraverser.batchSize()` should be changed to **really** honor the `estimateDepth` of `StackWalker.getInstance(.., estimateDepth)` (currently it is always `SMALL_BATCH` == `8` even if the caller specifies a smaller number): - int initialBatchSize = Math.max(walker.estimateDepth(), SMALL_BATCH); + int initialBatchSize = Math.max(walker.estimateDepth() + FrameBuffer.START_POS, MIN_BATCH_SIZE); - In `CallerClassFinder.initFrameBuffer()` you can then use `this.frameBuffer = new ClassFrameBuffer(walker, FrameBuffer.START_POS + 2)`. With these changes you would: - save one more frame for the `getCallerClass()` case - save more frames for any user supplied `estimateDepth` values smaller than `8` - don't implicitly expose `FrameBuffer.START_POS` to user space. Currently the user supplied `estimateDepth` value will be implicitly subtracted by `FrameBuffer.START_POS` - make all the internal size computations depend explicitly on `FrameBuffer.START_POS` for better maintainability. src/java.base/share/classes/java/lang/StackStreamFactory.java line 758: > 756: * So start the initial batch size with the minimum size. > 757: * If it fetches the second batch, getCallerClass may be invoked > via > 758: * core reflection, can increase the next batch size. This sentence reads strange to me. Maybe "If it fetches the second batch, getCallerClass may be invoked via core reflection, *so the next batch size will be increased.*"? ------------- Changes requested by simonis (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15642#pullrequestreview-1619856398 PR Review Comment: https://git.openjdk.org/jdk/pull/15642#discussion_r1321463264
