8304089: Convert TraceDependencies to UL #13007

coleenp · 2023-03-13T21:32:14Z

This change converts TraceDependencies to UL and removes the develop option. I think this provides further flexibility to add tags to only trace certain things in dependency analysis, as I did when trying to understand a PR for a deoptimization change. For now, the messages are the same and the option is -Xlog:dependencies=debug.
Tested with tier1-4

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8304089: Convert TraceDependencies to UL

Reviewers

Vladimir Ivanov (@iwanowww - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13007

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

Using diff file

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

bridgekeeper · 2023-03-13T21:33:33Z

👋 Welcome back coleenp! 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 · 2023-03-13T21:36:35Z

@coleenp The following label will be automatically applied to this pull request:

hotspot

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

mlbridge · 2023-03-14T13:28:28Z

Webrevs

iwanowww

Looks good.

openjdk · 2023-03-14T18:07:12Z

@coleenp 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:

8304089: Convert TraceDependencies to UL

Reviewed-by: vlivanov, dholmes

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 91 new commits pushed to the master branch:

0156909: 8304502: Classfile API class hierarchy makes assumptions when class is not resolved
0deb648: 8290200: com/sun/jdi/InvokeHangTest.java fails with "Debuggee appears to be hung"
019fcd8: 8304139: Add and method constants to ConstantDescs
d6f20e2: 8304680: Problemlist compiler/sharedstubs/SharedStubToInterpTest.java
d788a1b: 8304180: Constant Descriptors for MethodHandles::classData and classDataAt
bbde215: 8299494: Test vmTestbase/nsk/stress/except/except011.java failed: ExceptionInInitializerError: target class not found
1c04686: 8304387: Fix positions of shared static stubs / trampolines
c65bb2c: 8304334: java/awt/color/ICC_ColorSpace/ToFromCIEXYZRoundTrip.java times out on slow platforms
4bf1fbb: 8303648: Add String.indexOf(String str, int beginIndex, int endIndex)
c4df9b5: 8304537: Ant-based langtools build fails after JDK-8015831 Add lint check for calling overridable methods from a constructor
... and 81 more: https://git.openjdk.org/jdk/compare/55aa122462c34d8f4cafa58f4d1f2d900449c83e...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.

coleenp · 2023-03-14T19:32:19Z

Thank you Vladimir.

dholmes-ora

Overall looks good. Just a couple of small tweaks needed.

Thanks.

dholmes-ora · 2023-03-15T02:49:54Z

src/hotspot/share/runtime/arguments.cpp

@@ -3586,7 +3586,7 @@ static bool use_vm_log() {
  if (LogCompilation || !FLAG_IS_DEFAULT(LogFile) ||
      PrintCompilation || PrintInlining || PrintDependencies || PrintNativeNMethods ||
      PrintDebugInfo || PrintRelocations || PrintNMethods || PrintExceptionHandlers ||
-      PrintAssembly || TraceDeoptimization || TraceDependencies ||
+      PrintAssembly || TraceDeoptimization || log_is_enabled(Debug, dependencies) ||


Now TraceDependencies is converted to UL I think it should just be deleted from this function. We don't need to enable LogVMOutput in that case.

No, this is the right thing to do. If -Xlog:dependency - the compiler group also expects the dependency printed to the compiler log file. The logging is a separate mechanism, but should be enabled with -Xlog:dependency.

Sorry I don't follow that. use_vm_log() only affects non-product builds and forces LogVMOutput to true. That in turn will cause defaultStream::init_log() to execute which initializes the log file etc. But I don't see how that would cause UL logging for "dependencies" to also get written to the log file???

See the function log_dependency()

jdk/src/hotspot/share/code/dependencies.cpp

Line 845 in 421b4ee

void Dependencies::DepStream::log_dependency(Klass* witness) {

called from here (as one place).

jdk/src/hotspot/share/code/dependencies.cpp

Line 2071 in 421b4ee

log_dependency(witness);

When TraceDependencies was true, the log file would be non-null and log_dependency would write to it. Keeping this with -Xlog:dependencies=debug retains what TraceDependencies did.

Hmmm this seems broken to me then. We output the general dependency logging to one place based on UL configuration, but then we output log_dependency to the log file. If these are meant to be related and always reported together then we have lost that. If they are actually unrelated then this may still need the TraceDependencies flag to control it.

@iwanowww can you comment on this please?

Maybe we don't need this -Xlog test in this place, because this is also what PrintDependencies does.

Yes, UL check doesn't help here. Irrespective of the check, UL output isn't automatically placed in VM log.
It's convenient for LogCompilation to have VM output along with other VM events present in the log.
Having said that, I'm fine with dropping TraceDependencies check here and improve the interaction between UL and LogVMOutput separately.

Okay thank you.

src/hotspot/share/runtime/arguments.cpp

dholmes-ora

Nothing further from me - update is good. Thanks.

coleenp · 2023-03-22T12:31:31Z

Thank you Vladimir and David.
/integrate

openjdk · 2023-03-22T12:33:09Z

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

358c61b: 8294972: Convert jdk.jlink internal plugins to use the Classfile API
c74507e: 8304657: G1: Rename set_state_empty to set_state_untracked
ca94287: 8304144: G1: Remove unnecessary is_survivor check in G1ClearCardTableTask
eda0065: 8304286: java/net/SocketOption/OptionsTest.java failing after JDK-8302659
c039d26: 8303804: Fix some errors of If-VectorTest and CMove-VectorTest
0156909: 8304502: Classfile API class hierarchy makes assumptions when class is not resolved
0deb648: 8290200: com/sun/jdi/InvokeHangTest.java fails with "Debuggee appears to be hung"
019fcd8: 8304139: Add and method constants to ConstantDescs
d6f20e2: 8304680: Problemlist compiler/sharedstubs/SharedStubToInterpTest.java
d788a1b: 8304180: Constant Descriptors for MethodHandles::classData and classDataAt
... and 86 more: https://git.openjdk.org/jdk/compare/55aa122462c34d8f4cafa58f4d1f2d900449c83e...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-22T12:33:24Z

@coleenp Pushed as commit ddf1e34.

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

Logging

d42c4a3

openjdk bot added the hotspot hotspot-dev@openjdk.org label Mar 13, 2023

coleenp changed the title ~~Logging~~ 8304089: Convert TraceDependencies to UL Mar 14, 2023

coleenp marked this pull request as ready for review March 14, 2023 13:22

openjdk bot added the rfr Pull request is ready for review label Mar 14, 2023

coleenp added 2 commits March 14, 2023 13:33

Merge branch 'master' into trace-dependencies-logging

8c0bc89

Fix merge conflict.

e585120

iwanowww approved these changes Mar 14, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 14, 2023

dholmes-ora suggested changes Mar 15, 2023

View reviewed changes

coleenp added 2 commits March 15, 2023 13:02

Fix redundant 'if'

421b4ee

Drop UL check for log file output (still have PrintDependencies).

c078451

iwanowww approved these changes Mar 21, 2023

View reviewed changes

dholmes-ora approved these changes Mar 22, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 22, 2023

openjdk bot closed this Mar 22, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 22, 2023

coleenp deleted the trace-dependencies-logging branch March 22, 2023 12:39

8304089: Convert TraceDependencies to UL #13007

8304089: Convert TraceDependencies to UL #13007

Uh oh!

Conversation

coleenp commented Mar 13, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 13, 2023

Uh oh!

openjdk bot commented Mar 13, 2023

Uh oh!

mlbridge bot commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coleenp commented Mar 14, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

coleenp Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

coleenp Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

coleenp Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

iwanowww Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

coleenp Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp commented Mar 22, 2023

Uh oh!

openjdk bot commented Mar 22, 2023

Uh oh!

openjdk bot commented Mar 22, 2023

Uh oh!

Uh oh!

coleenp commented Mar 13, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 14, 2023 •

edited

Loading

openjdk bot commented Mar 14, 2023 •

edited

Loading

coleenp Mar 15, 2023 •

edited

Loading