8300273: [IR framework] Handle <!-- safepoint while printing --> message instead of bailing out

[chhagedorn]

This is yet another attempt to fix the IR framework in cases where the tty lock is broken for a safepoint. This time, however, I've decided to not add another bailout but to just rewrite the hotspot_pid* parsing code to completely parse everything. This means that <!-- safepoint while printing --> messages are now skipped and then parsing continues for the reminder of a PrintIdeal output block.

When are tty locks broken?

This can only happen when dumping PrintIdeal for various compile phases. For PrintOptoAssembly, we have a NoSafePointVerifier object that ensures that we are not safepointing during the dump:

jdk/src/hotspot/share/opto/output.cpp

Lines 1814 to 1822 in f7da09c

	NoSafepointVerifier nsv;
	ttyLocker ttyl2;
	// This output goes directly to the tty, not the compiler log.
	// To enable tools to match it up with the compilation activity,
	// be sure to tag this tty output with the compile ID.
	if (xtty != NULL) {
	xtty->head("opto_assembly compile_id='%d'%s", C->compile_id(),
	C->is_osr_compilation() ? " compile_kind='osr'" : "");
	}

The last interesting message printed by LogCompilation that needs to be parsed is the compilation queued message. This message is emitted in CompileTask::log_task_queued() where we also take the tty lock. Since we do not seem to safepoint during this method, I've added a NoSafepointVerifier object - just to be sure.

Implementation details

The idea is that we keep track of writer thread ids and currently parsed PrintIdeal blocks when parsing the hotspot_pid* file. If a new writer thread starts writing, it emits a <writer thread='1683670'/> message with the id. If that happens while we are still parsing a PrintIdeal block (i.e. not found the </ideal> end tag for the <ideal ... /> opening tag), we store the currently parsed method (i.e. a LoggedMethod object) with its WriterThread in WriterThread::saveLoggedMethod(). When later parsing the same writer id again, we restore the method with WriterThread::restoreLoggedMethod() and continue parsing.

Each line of a PrintIdeal output block is stored within a CompilePhaseBlock object. This class makes sure to remove any <!-- safepoint while printing --> message (always at the end of a line with a line break) and to merge such a split line again.

I've removed all previously added bailout fixes as they are not needed anymore with this fix.

I've added some more classes and did some renamings to better structure the parsing steps.

Testing

On top of normal tier testing, I've added a separate test that parses a manually written hotspot_pid* file that simulates various cases of breaking the tty log and emitting the <!-- safepoint while printing --> message with a line break.

Thanks,
Christian

---

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-8300273: [IR framework] Handle <!-- safepoint while printing --> message instead of bailing out

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12246

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

Using diff file

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

[bridgekeeper]

👋 Welcome back chagedorn! 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]

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

• hotspot-compiler

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]

Webrevs

• 02: Full - Incremental (848e58e5)
• 01: Full - Incremental (612f81ae)
• 00: Full (ecab8028)

[TobiHartmann]

This is hard to review but the solution makes sense and the code looks good to me. Some of the files still have copyright year 2022.

[openjdk]

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

8300273: [IR framework] Handle <!-- safepoint while printing --> message instead of bailing out

Reviewed-by: thartmann, kvn

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

• 24ff3da: 8301201: Allow \n@ inside inline tags using inlineContent
• 3361a26: 8298874: Update TestAllSuites.java for TLS v1.2 and 1.3
• bc750f7: 8294527: Some java.security.debug options missing from security docs
• 7c6a8db: 8301447: [REDO] CodeHeap has virtual methods that are not overridden
• cae577a: 8295486: Inconsistent constant field values observed during compilation
• 969f6a3: 8301093: C2 fails assert(ctrl == kit.control()) failed: Control flow was added although the intrinsic bailed out
• 2a8ae2f: 8300256: C2: vectorization is sometimes skipped on loops where it would succeed
• ef0d0a7: 8301402: os::print_location gets is_global_handle assert
• 4f6f3cc: 8301446: Remove unused includes of gc/shared/genOopClosures
• a0aed9b: 8301459: Serial: Merge KeepAliveClosure into FastKeepAliveClosure
• ... and 43 more: https://git.openjdk.org/jdk/compare/3db558b67bebfe559833331475f481c588147084...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.

[chhagedorn]

Thank you Tobias for your review! I've updated the copyright years.

[vnkozlov]

"Rubber stamp" ;^)

[chhagedorn]

Thanks Vladimir for your review :-)

I gave this another spin in the Valhalla repository and noticed that I accidentally dropped the XML escaping. I've re-added it. Will re-run some testing again.

[vnkozlov]

Update is good.

[chhagedorn]

Thanks Tobias and Vladimir for your reviews! Valhalla testing looked good with the update.

/integrate

[openjdk]

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

• 5b1584b: 8298880: VectorLogicalOpIdentityTest.java IR test incorrectly use avx3 instead of avx512
• 21c1afb: 8301612: OopLoadProxy constructor should be explicit
• 03b23a1: 8301367: Add exception handler method to the BaseLdapServer
• 7b6ac41: 8286876: NMT.test_unaliged_block_address_vm_assert fails if using clang toolchain
• d097b5e: 8301508: Replace NULL with nullptr in os_cpu/linux_s390
• c8307e3: 8301500: Replace NULL with nullptr in os_cpu/aix_ppc
• 218223e: 8301501: Replace NULL with nullptr in os_cpu/bsd_aarch64
• b81f0ff: 8301505: Replace NULL with nullptr in os_cpu/linux_arm
• 42a286a: 8301511: Replace NULL with nullptr in os_cpu/linux_zero
• ad79e49: 8301512: Replace NULL with nullptr in os_cpu/windows_aarch64
• ... and 69 more: https://git.openjdk.org/jdk/compare/3db558b67bebfe559833331475f481c588147084...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn Pushed as commit 59b7fb1.

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