On Wed, 23 Sep 2020 15:45:12 GMT, Albert Mingkun Yang <ay...@openjdk.org> wrote:
>> Marked as reviewed by stefank (Reviewer). > > Given names like `StackWatermarkSet::lowest_watermark`, I wonder if some > diagrams could be provided in the code > comments for `class StackWatermarks` so that readers will have the correct > mental pictures of the layout of stack > frames and watermarks. Some docs on `volatile uint32_t _state;` in > `StackWatermark` would be nice, such as what info > is encoded in this state, typical state transitions, who can access this > state (tying it to volatile) etc. > I wonder if it's possible to remove `bool _is_done;` in > `StackWatermarkFramesIterator`; just calling `is_done()` on the > underlying frame stream seems cleaner. Thanks for reviewing Albert. > Given names like `StackWatermarkSet::lowest_watermark`, I wonder if some > diagrams could be provided in the code > comments for `class StackWatermarks` so that readers will have the correct > mental pictures of the layout of stack > frames and watermarks. Added some further comments and diagrams. > Some docs on `volatile uint32_t _state;` in `StackWatermark` would be nice, > such as what info is encoded in this state, > typical state transitions, who can access this state (tying it to volatile) > etc. Added some comments about the state. > I wonder if it's possible to remove `bool _is_done;` in > `StackWatermarkFramesIterator`; just calling `is_done()` on the > underlying frame stream seems cleaner. Unfortunately this is not possible. The underlying frame stream is_done() function is not idempotent. It alters the state of the iteration as a side effect of asking the question whether it is done or not. The first time it reaches past the last frame it will report true, and the second time it will report false. So for that use to be valid, it is assumed that you only ask if you are is_done() once. But the way I use it, I ask this question potentially more than once after we are done, and expect it to return false consistently. That's why I read it once from the stream and cache the result that I use. One might argue that the underlying stream class has undesirable behaviour, but fixing that seemed outside the scope of this RFE. I have had too many rabbit holes already. ------------- PR: https://git.openjdk.java.net/jdk/pull/296
