On Thu, 4 May 2023 13:36:22 GMT, Tobias Holenstein <tholenst...@openjdk.org>
wrote:
> At the moment `CompileCommand` and `CompileOnly` use different syntax for
> matching methods.
>
> ### Old CompileOnly format
> - matching a **method name** with **class name** and **package name**:
> `-XX:CompileOnly=package/path/Class.method`
> `-XX:CompileOnly=package/path/Class::method`
> `-XX:CompileOnly=package.path.Class::method`
> BUT NOT `-XX:CompileOnly=package.path.Class.method`
>
> - just matching a **single method name**:
> `-XX:CompileOnly=.hashCode`
> `-XX:CompileOnly=::hashCode`
> BUT NOT `-XX:CompileOnly=hashCode`
>
> - Matching **all method names** in a **class name** with **package name**
> `-XX:CompileOnly=java/lang/String`
> BUT NOT `-XX:CompileOnly=java/lang/String.`
> BUT NOT `-XX:CompileOnly=java.lang.String`
> BUT NOT `-XX:CompileOnly=java.lang.String::` (This is actually a bug)
> BUT NOT `-XX:CompileOnly=String`
> BUT NOT `-XX:CompileOnly=String.`
> BUT NOT `-XX:CompileOnly=String::`
>
> - Matching **all method names** in a **class name** with **NO package name**
> `-XX:CompileOnly=String`
> BUT NOT `-XX:CompileOnly=String.`
> BUT NOT `-XX:CompileOnly=String::`
>
> - There is a bug when `CompileOnly` ends with `::` where the `CompileOnly` is
> just ignored
> e.g. `-XX:CompileOnly=String::` compiles as many methods as when omitting the
> `-XX:CompileOnly=` command
>
> ### CompileCommand=compileonly format
> `CompileCommand` allows two different forms for paths:
> - `package/path/Class.method`
> - `package.path.Class::method`
>
> In contrary to `CompileOnly` `CompileCommand` supports wildcard matching
> using `*`. `*` can appear at the beginning and/or end of a
> `package.path.Class` and `method` name.
>
> Valid forms:
> `-XX:CompileCommand=compileonly,*.lang.*::*shCo*`
> `-XX:CompileCommand=compileonly,*/lang/*.*shCo*`
> `-XX:CompileCommand=compileonly,java.lang.String::*`
> `-XX:CompileCommand=compileonly,*::hashCode`
> `-XX:CompileCommand=compileonly,*ng.String::hashC*`
> `-XX:CompileCommand=compileonly,*String::hash*`
>
> Invalid forms (Error: Embedded * not allowed):
> `-XX:CompileCommand=compileonly,java.*.String::has*Code`
>
> ### Use CompileCommand syntax for CompileOnly
> At the moment, in some cases it is not possible to just take pattern used
> with `CompileOnly` and plug it into compile command file. Syntax used by
> CompileOnly is also not very intuitive.
>
> `CompileOnly` is convenient because it's shorter to write and takes lists of
> patterns, whereas `CompileCommand` only takes one pattern per command.
>
> With this PR `CompileOnly` becomes an alias for `CompileC...
Nice unification! I just have some small comments. Otherwise, looks good!
src/hotspot/share/compiler/compilerOracle.cpp line 1015:
> 1013:
> 1014: void CompilerOracle::parse_compile_only(char* line) {
> 1015: if (line[0] == '\0') return;
I suggest to use braces for single line ifs:
Suggestion:
if (line[0] == '\0') {
return;
}
src/hotspot/share/compiler/compilerOracle.cpp line 1017:
> 1015: if (line[0] == '\0') return;
> 1016: ResourceMark rm;
> 1017: char error_buf[1024] = {0};
Wouldn't it be sufficient to only initialize the first character with `\0`?
src/hotspot/share/compiler/compilerOracle.cpp line 1021:
> 1019: char* method_pattern;
> 1020: do {
> 1021: if (line[0] == '\0') break;
Suggestion:
if (line[0] == '\0') {
break;
}
test/hotspot/jtreg/compiler/loopopts/Test8211698.java line 57:
> 55: }
> 56: }
> 57:
You can leave this empty line in at the end of the file.
-------------
Marked as reviewed by chagedorn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13802#pullrequestreview-1438882451
PR Review Comment: https://git.openjdk.org/jdk/pull/13802#discussion_r1201705460
PR Review Comment: https://git.openjdk.org/jdk/pull/13802#discussion_r1201720761
PR Review Comment: https://git.openjdk.org/jdk/pull/13802#discussion_r1201705793
PR Review Comment: https://git.openjdk.org/jdk/pull/13802#discussion_r1201731777
