RFR: 8342577: Clean up JVMTI breakpoint support

[Chris Plummer]

On Thu, 24 Oct 2024 01:42:36 GMT, Alex Menkov <amenkov at 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