On Thu, 24 Oct 2024 01:42:36 GMT, Alex Menkov <amen...@openjdk.org> wrote:
> The fix cleans up code to support list of JVMTI breakpoints.
> - classes required to supports cache of byte code pointers (GrowableElement,
> GrowableCache, JvmtiBreakpointCache) are dropped;
> - class JvmtiCurrentBreakpoints (JvmtiBreakpoints factory) is left as is,
> dropped unused code;
> - fixed race in JvmtiCurrentBreakpoints::get_jvmti_breakpoints() (fix for
> JDK-8210637);
> - JvmtiBreakpoint:JvmtiBreakpoint() + JvmtiBreakpoint::copy(JvmtiBreakpoint&
> bp) are replaced with copy ctor;
> - JvmtiBreakpoints::clearall_in_class_at_safepoint() is simplified to do a
> single pass;
>
> Testing: tier1..tier6
Overall looks good. I just made a testing comment and a general comment about
the optimization you did, but no need for changes.
src/hotspot/share/prims/jvmtiImpl.cpp line 208:
> 206:
> 207: JvmtiBreakpoints::JvmtiBreakpoints()
> 208: : _elements(5, mtServiceability) {
Do we have tests that create more than 5 breakpoints? I just want to make sure
the array growing code is exercised. You could stress it by testing with the
initial size set to 1.
src/hotspot/share/prims/jvmtiImpl.cpp line 276:
> 274: if (bp.method()->method_holder() == klass) {
> 275: bp.clear();
> 276: remove(i);
After reading the original comments, I get the feeling that the concern was
that the array might be re-organized (recached). The comment "...the next entry
might no longer be at i+1" of course is true. It would be at i if there was no
recaching, in which case you could use while loop starting at 0 and not
increment `i` if the entry is deleted.
-------------
Marked as reviewed by cjplummer (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21675#pullrequestreview-2400686253
PR Review Comment: https://git.openjdk.org/jdk/pull/21675#discussion_r1820068812
PR Review Comment: https://git.openjdk.org/jdk/pull/21675#discussion_r1820079694
