[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: RFR: 8300273: [IR framework] Handle <!-- safepoint while printing --> message instead of bailing
From:       Christian Hagedorn <chagedorn () openjdk ! org>
Date:       2023-01-31 15:54:14
Message-ID: DC-XiC2oMtK6QYQ_baZ51ZBAMGGye1bXsKGY1b-9_1I=.9d63345d-074f-403f-8be2-ea4ae517caa7 () github ! com
[Download RAW message or body]

On Mon, 30 Jan 2023 09:04:21 GMT, Christian Hagedorn <chagedorn@openjdk.org> wrote:

> > 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 `` 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: \
> > https://github.com/openjdk/jdk/blob/f7da09c34918eea434c82af22b1da1f2a5b35f35/src/hotspot/share/opto/output.cpp#L1814-L1822
> >  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 `` 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 `` message with a line break. 
> > Thanks,
> > Christian
> 
> Christian Hagedorn has updated the pull request incrementally with one additional \
> commit since the last revision: 
> Update copyright years

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.

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

PR: https://git.openjdk.org/jdk/pull/12246


[prev in list] [next in list] [prev in thread] [next in thread]