Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8027711: Unify wildcarding syntax for CompileCommand and CompileOnly #13802

Closed
wants to merge 20 commits into from

Conversation

tobiasholenstein
Copy link
Contributor

@tobiasholenstein tobiasholenstein commented May 4, 2023

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 CompileCommand=compileonly with possibility to take lists as input.

New syntax for CompileOnly:

  • -XX:CompileOnly=pattern1,pattern2
    is an alias for:
  • -XX:CompileCommand=compileonly,pattern1 -XX:CompileCommand=compileonly,pattern2

Handling invalid syntax

Before CompileOnly just ignored invalid syntax. CompileCommand at least prints an error massage for invalid patterns:

CompileOnly: An error occurred during parsing
Error: Could not parse method pattern
Line: 'pattern'

Since CompileOnly now maps to CompileCommand is also prints the error message for invalid inputs.
In the future CompileCommand (and CompileOnly) parsing errors could exit the VM https://bugs.openjdk.org/browse/JDK-8282797

Changed test cases

In the following we mean with -XX:CompileOnly=oldPattern -> -XX:CompileOnly=newPattern that we changed the tests from the oldPattern to the newPattern:

  • Class.method -> Class::method AND package/path/Class.method -> package.path.Class::method
    Prefer the package.path.Class::method format because it is used by -XX:+PrintCompilation

  • Class -> Class::*
    The CompileCommand format requires the :: to define if Class is a class name or a method name.

  • ::method -> *::method
    The CompileCommand format requires the *(wildcard) if no class name is given.

  • package/path/Class -> package.path.Class::*
    The CompileCommand format requires the *(wildcard) if no method name is given.
    Prefer the package.path.Class::method format because it is used by -XX:+PrintCompilation

  • ::get,::get1 -> *Class::get*
    The CompileCommand format requires the *(wildcard) if no method name is given. Therefore *::get would work as well, but this matches many other methods as well like java.util.HashMap::get.
    *Class::get,*Class::get1 matches the wanted class name - *Class::get* is just the short form.

  • package/path/Class::method -> package.path.Class::method
    The old format of CompileOnly combining / with ::.
    The CompileCommand is either package/path/Class.method or package.path.Class::method.

  • BUG: package.path.Class:: -> package.path.Class::*
    There was a bug in the old format of CompileOnly : when the pattern ended with :: the CompileOnly was just ignored and all methods compiled. The CompileCommand format requires the *(wildcard) if no method name is given.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change requires CSR request JDK-8308287 to be approved
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issues

  • JDK-8027711: Unify wildcarding syntax for CompileCommand and CompileOnly (Enhancement - P4) ⚠️ Issue is already resolved. Consider making this a "backport pull request" by setting the PR title to Backport <hash> with the hash of the original commit. See Backports.
  • JDK-8308287: Unify syntax of CompileOnly and CompileCommand (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13802/head:pull/13802
$ git checkout pull/13802

Update a local copy of the PR:
$ git checkout pull/13802
$ git pull https://git.openjdk.org/jdk.git pull/13802/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13802

View PR using the GUI difftool:
$ git pr show -t 13802

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13802.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 4, 2023

👋 Welcome back tholenstein! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented May 4, 2023

@tobiasholenstein The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added hotspot-compiler hotspot-compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels May 4, 2023
Update TestPinnedNodeInInnerLoop.java

Update TestStoreSunkToOuterLoop.java

Update DeadNodesInOuterLoopAtLoopCloning2.java

Update VolatileLoadMemBarsOnlyUses.java

Update TestStableBoolean.java

Update TestStableByte.java

Update TestStableChar.java

Update TestStableDouble.java

Update TestStableFloat.java

Update TestStableInt.java

Update TestStableLong.java

Update TestStableMemoryBarrier.java

Update TestStableMismatched.java

Update TestStableObject.java

Update TestStableUByte.java

Updated TestStableUShort.java

Update TestStableShort.java

Update TestPickLastMemoryState.java

Update TestDivWithTopDivisor.java

Update TestNegBaseOffset.java

Update TestMainBodyExecutedOnce.java

Update TestMainNeverExecuted.java

Update TestCastIIMakesMainLoopPhiDead2.java

Update TestDivZeroWithSplitIf.java

Update TestUnreachableInnerLoop.java

Update TestCastIIMakesMainLoopPhiDead.java

Update TestDivZeroDominatedBy.java

Update TestDeadPostLoopBecausePredicate.java

Update TestBadlyFormedCountedLoop.java

Update TestAddPChainWithDifferentBase.java

Update Test8211698.java

Update TestSunkNodeDueToBrokenAntiDependency.java

Updated TestCountedLoopZeroIter.java

Update TestCastFFAtPhi.java

Update TestStoreSunkInInnerLoop.java

Update PeelingZeroTripCount.java

Update TestLoopLimitNodeElimination.java

Update TestCastIIAfterUnrollingInOuterLoop.java

Update UnexpectedPinnedNodeInOuterLoop.java

Update UnexpectedNodeInOuterLoopWhenCloning.java

Update TestUseFromInnerInOuterUnusedBySfpt.java

update TestSHA512Intrinsics.java

Update PrintIdealPhaseTest.java

Update TestSqrt.java

Update TestCMoveHasTopInput.java

Update TestModDivTopInput.java

Update TestMatcherLargeOffset.java

Update TestCondAddDeadBranch.java

Update TestMD5MultiBlockIntrinsics.java

Update TestSHA3MultiBlockIntrinsics.java

Update TestSHA512MultiBlockIntrinsics.java

Update TestMD5Intrinsics.java

Update TestSHA1MultiBlockIntrinsics.java

Update TestSHA256Intrinsics.java

Update TestSHA3Intrinsics.java

Update TestSHA512Intrinsics.java

Update TestSHA1Intrinsics.java

Update TestFpMinMaxIntrinsics.java + 1

atom TestSmallVectorPopIndex.java

Update TestGCMStorePlacement.java

Updated TestArrayCopyToFromObject.java

Updated UnsignedLoads.java +3

updated tests in jdk/internal/vm/Continuation

Update OptionTest.java

Update OptionTest.java

revert PrintIdealPhaseTest.java

Update OptionTest.java

Update OptionTest.java
@tobiasholenstein
Copy link
Contributor Author

/csr needed

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label May 17, 2023
@openjdk
Copy link

openjdk bot commented May 17, 2023

@tobiasholenstein has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@tobiasholenstein please create a CSR request for issue JDK-8027711 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

@tobiasholenstein tobiasholenstein marked this pull request as ready for review May 17, 2023 14:34
@openjdk openjdk bot added the rfr Pull request is ready for review label May 17, 2023
@tobiasholenstein
Copy link
Contributor Author

The CSR can be found here: https://bugs.openjdk.org/browse/JDK-8308287

@mlbridge
Copy link

mlbridge bot commented May 17, 2023

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this finally!

FTR. We planned to do this for long time. Main motivations: unify syntax and catch invalid commands.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this tedious changes, Toby. Looks good to me too!

Copy link
Member

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice unification! I just have some small comments. Otherwise, looks good!

src/hotspot/share/compiler/compilerOracle.cpp Outdated Show resolved Hide resolved
src/hotspot/share/compiler/compilerOracle.cpp Outdated Show resolved Hide resolved
src/hotspot/share/compiler/compilerOracle.cpp Show resolved Hide resolved
test/hotspot/jtreg/compiler/loopopts/Test8211698.java Outdated Show resolved Hide resolved
tobiasholenstein and others added 3 commits May 23, 2023 11:01
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Copy link
Member

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update looks good!

@tobiasholenstein
Copy link
Contributor Author

Thanks for the reviews @vnkozlov , @chhagedorn and @TobiHartmann

@openjdk
Copy link

openjdk bot commented Jun 9, 2023

@tobiasholenstein This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8027711: Unify wildcarding syntax for CompileCommand and CompileOnly

Reviewed-by: kvn, thartmann, chagedorn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 15 new commits pushed to the master branch:

  • 16c3d53: 8308603: Removing do_pending_ref/enclosing_ref from MetaspaceClosure
  • b94b679: 8309627: Incorrect sorting of DirtyCardQueue buffers
  • aace3dc: 8309760: ProblemList serviceability/jvmti/vthread/FollowReferences/VThreadStackRefTest.java#default with ZGC
  • 80edd5c: 8294985: SSLEngine throws IAE during parsing of X500Principal
  • bdd81b3: 8304885: Reuse stale data to improve DNS resolver resiliency
  • beec734: 8309692: Fix -Wconversion warnings in javaClasses
  • 7d82479: 8309142: Refactor test/langtools/tools/javac/versions/Versions.java
  • f5ec93e: 8309745: Problem list open client tests failing on Ubuntu_23.04
  • cee5724: 8309727: Assert privileges while reading the jdk.incubator.vector.VECTOR_ACCESS_OOB_CHECK system property
  • 7d6f97d: 8309673: Refactor ref_at methods in SA ConstantPool
  • ... and 5 more: https://git.openjdk.org/jdk/compare/c052756154603a9d3a13200fa407a2dc124437f3...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Jun 9, 2023
@vnkozlov
Copy link
Contributor

vnkozlov commented Jun 9, 2023

GHA testing failed. Please look.

CompileOnly: An error occurred during parsing
Error: Could not parse method pattern
Line: 'TestZeroTripGuardShared'

Copy link
Member

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update and testing results look good now!

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looks good.

@tobiasholenstein
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jun 12, 2023

Going to push as commit f5cbe53.
Since your change was applied there have been 17 commits pushed to the master branch:

  • 4d66d97: 8309549: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java fails on AIX
  • 3981297: 8309703: AIX build fails after JDK-8280982
  • 16c3d53: 8308603: Removing do_pending_ref/enclosing_ref from MetaspaceClosure
  • b94b679: 8309627: Incorrect sorting of DirtyCardQueue buffers
  • aace3dc: 8309760: ProblemList serviceability/jvmti/vthread/FollowReferences/VThreadStackRefTest.java#default with ZGC
  • 80edd5c: 8294985: SSLEngine throws IAE during parsing of X500Principal
  • bdd81b3: 8304885: Reuse stale data to improve DNS resolver resiliency
  • beec734: 8309692: Fix -Wconversion warnings in javaClasses
  • 7d82479: 8309142: Refactor test/langtools/tools/javac/versions/Versions.java
  • f5ec93e: 8309745: Problem list open client tests failing on Ubuntu_23.04
  • ... and 7 more: https://git.openjdk.org/jdk/compare/c052756154603a9d3a13200fa407a2dc124437f3...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 12, 2023
@openjdk openjdk bot closed this Jun 12, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 12, 2023
@openjdk
Copy link

openjdk bot commented Jun 12, 2023

@tobiasholenstein Pushed as commit f5cbe53.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
4 participants