On Tue, 23 May 2023 17:19:23 GMT, Vladimir Ivanov <vliva...@openjdk.org> wrote:
>>> I verified that the new test cases do trigger SR+NSR scenario.
>>>
>>> How do you test that deoptimization works as expected?
>>>
>>
>> I have a copy of the tests in AllocationMergesTests.java in a separate file
>> (not included in this PR) and I run the tests with a tool that compares the
>> output of the test with RAM enabled and disabled. So, the way I test that
>> deoptimization worked is basically just making sure the tests that
>> "deoptimize" have the same output with RAM enabled and disabled.
>>
>>> Diagnostic output is still hard to read. On one hand, it's too verbose when
>>> it comes to PcDesc/ScopeDesc sections ("pc-bytecode offsets" and "scopes")
>>> in nmethod output (enabled either w/ `-XX:+PrintAssembly` or
>>> `-XX:CompileCommand=print,...`). On the other hand, it lacks some important
>>> details, like `selector` and `merge_ptr` location information which is
>>> essential to make sense of debug information at a safepoint in the code.
>>>
>>
>> I'll take care of that. I was testing only with PrintDebugInfo.
>>
>>> FTR `_skip_rematerialization` flag is unused now.
>>>
>>
>> yeah, I forgot to remove that. Thanks.
>>
>>> Speaking of `_only_merge_candidate` flag, I find it easier about the code
>>> when the property being tracked is whether the `ObjectValue` is referenced
>>> from corresponding JVM state or not. (Maybe call it `is_root()`?) So,
>>> `ScopeDesc::objects_to_rematerialize()` would skip everything not
>>> referenced from JVM state, but then unconditionally accept anything
>>> returned by `ObjectMergeValue::select()` which doesn't need to adjust the
>>> flag before returning selected object. Also, it's safer to track the flag
>>> status for every `ObjectValues`, even for `ObjectMergeValue`.
>>>
>>
>> Sounds like a good idea. I'll do that. Thanks.
>>
>>> Are you sure there's no way to end up with nested `ObjectMergeValue`s in
>>> presence of iterative EA?
>>
>> I don't think so. This current patch only handle Phis that don't have NULL
>> as input. As part of the reduction process we set at least one of the
>> reducible Phi inputs to NULL. Therefore, subsequent iterations of EA won't
>> reduce the same Phi.
>
>> So, the way I test that deoptimization worked is basically just making sure
>> the tests that "deoptimize" have the same output with RAM enabled and
>> disabled.
>
> Please, enhance `AllocationMergesTests` to cover deoptimization (e.g., using
> WhiteBox API or additional run w/ -XX:+DeoptimizeALot) and ensure that tests
> are sensitive enough to fail when wrong state is rematerialized.
@iwanowww - I want to clarify expectations with my colleagues so I have to ask
you how much left are there for you to review and whether there is some part of
this PR that you're worried about in terms of correctness/performance/etc?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/12897#issuecomment-1584907343
