RFR: 8354949: JFR: Split up the EventInstrumentation class

[Chen Liang]

On Thu, 17 Apr 2025 14:11:25 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

> Could I have a review of a PR that cleans up the EventInstrumentation class and moves the non-modifying parts into a separate class (ClassInspector)? 
> 
> I deleted the code that removes the instrumentation (buildUninstrumented), because it has never been used. If there should be a need for it in the future, it's not hard to add code that inserts empty method bodies.
> 
> Testing: tier1 + jdk/jdk/jfr/test
> 
> Thanks
> Erik

src/jdk.jfr/share/classes/jdk/jfr/internal/ClassInspector.java line 102:

> 100:         for (MethodModel method : classModel.methods()) {
> 101:             String d = method.methodTypeSymbol().descriptorString();
> 102:             if (method.methodName().equalsString("commit") && m.descriptor().descriptorString().equals(d)) {

I think we need a `MethodDesc.matches` to conveniently check a (name, methodTypeDesc) tuple matches a NameAndType entry or a MethodModel. This check is repeated a few times in this class.

src/jdk.jfr/share/classes/jdk/jfr/internal/ClassInspector.java line 145:

> 143:         for (MethodModel m : classModel.methods()) {
> 144:             if (m.methodName().equalsString(method.name()) && m.methodTypeSymbol().equals(method.descriptor())) {
> 145:                 return Modifier.isStatic(m.flags().flagsMask());

Nit: checking the static flag before checking the name might require less method name decoding

src/jdk.jfr/share/classes/jdk/jfr/internal/ClassInspector.java line 217:

> 215:         List<AnnotationValue> list = new ArrayList<>();
> 216:         for (Attribute<?> attribute: classModel.attributes()) {
> 217:             if (attribute instanceof RuntimeVisibleAnnotationsAttribute rvaa) {

`findAttribute` or `findAttributes` can be helpful if you just want one attribute.

src/jdk.jfr/share/classes/jdk/jfr/internal/ClassInspector.java line 291:

> 289:                             // Add setting if method returns boolean and has one parameter
> 290:                             MethodTypeDesc mtd = m.methodTypeSymbol();
> 291:                             if ("Z".equals(mtd.returnType().descriptorString())) {

`if (CD_boolean.equals(mtd.returnType()))` might be more straightforward.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24726#discussion_r2049384841
PR Review Comment: https://git.openjdk.org/jdk/pull/24726#discussion_r2049386739
PR Review Comment: https://git.openjdk.org/jdk/pull/24726#discussion_r2049388365
PR Review Comment: https://git.openjdk.org/jdk/pull/24726#discussion_r2049391888