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
8261137: Optimization of Box nodes in uncommon_trap #2401
Conversation
👋 Welcome back whuang! A progress list of the required criteria for merging this PR into |
@Wanghuang-Huawei The following label will be automatically applied to this pull request:
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. |
/contributor add Wu Yan wuyan34@huawei.com |
@Wanghuang-Huawei |
/contributor add Ai Jiaming aijiaming1@huawei.com |
@Wanghuang-Huawei |
@Wanghuang-Huawei |
Webrevs
|
I was wandering if we can remove the useless |
The method
|
Thanks for your explanation. This makes more sense to me now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little concern about stretching uses of input value outside scope where it is created (for example, loop's variable or a value depending on it).
This optimization may work only because boxed values are immutable.
I will run our tests with this changes.
Our tier1-4 testing passed |
Eventually, c2 should know that `i < data.length' is true. it should be done by GCP later. The example is a special case. you can remove this " if (i < data.length)" here. Generally, target code can look like this. c2 speculatively generates an uncommon_trap of "unstable_if".
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The improvement you are proposing is not specific to uncommon traps, but can be generalized to any debug usage at safepoints.
The downside is that, in general, rematerialization logic has to use the corresponding pure function in order to materialize the eliminated instance. In this particular case (primitive boxing), it has to take into account the caching effects of primitive box factories. Otherwise, user code can encounter identity paradoxes with rematerialized primitive box instances.
I don't see how the scalarization logic you propose preserves identity constraints imposed by valueOf
factories.
Yes, it seems this optimization introduces the issue we had with Graal (8223320): |
In addition to using the logic that we already have there for Graal (see |
Also, I don't know if EA can handle a case when an Integer that is coming from an allocation and from a valueOf() are both inputs to a phi.
If it does, then we'd need to force materialization. |
Currently C2 EA can't scalarize merged allocations. So this is not an issue for now but may be in a future. Also C2 does not replace valueOf() with scalar node - it inlines it. As result you have branches with allocation and load from cache. Only this patch propose to use scalar node for valueOf() for the first time in C2. But it is replaced only in case it is directly referenced by debug info (no Phi or other nodes in between). So I think it is safe if we use AutoBoxObjectValue for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add test case which verifies that Box is scalarized by forking process and checking output of run with -XX:+PrintEliminateAllocations
flag.
You also need a test which triggers deoptimization and execute code for Box object reallocation/initialization or load from cache.
A test should also verifies that box object identity matches after deoptimization in case when object is loaded from cache.
Okay. I will do it for 8263125 fix. |
I added both flags checks in deoptimizer in #2924 But I am lost on the status of these changes. Do you have other updates for them? Or current 05 is final? |
OK. Thank you for your work. |
@iwanowww Can you review my new 06 version? I have split |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks good.
Some refactoring suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Some more minor cleanup suggestions follow.
|
||
static void scalarize_debug_usages(CallNode* call, Node* resproj) { | ||
Unique_Node_List safepoints; | ||
for (DUIterator_Fast imax, i = resproj->fast_outs(imax); i < imax; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to collect safepoints anymore. You can process users while iterating over them.
for (DUIterator_Last imin, i = res->last_outs(imin); i >= imin;) {
SafePointNode* sfpt = res->last_out(i)->as_SafePoint();
...
int num_edges = sfpt->replace_edges_in_range(res, sobj, start, end);
i -= num_edges;
}
} | ||
|
||
#ifndef PRODUCT | ||
if (PrintEliminateAllocations && safepoints.size() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
safepoints.size() > 0
is redundand.
Also, I'd move the diagnostic logic to the end of the method after all the users are processed.
ProjNode* res = resproj->as_Proj(); | ||
Node* sfpt = safepoints.pop(); | ||
|
||
ciInstanceKlass* klass = call->as_CallStaticJava()->method()->holder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
klass
is loop invariant and can be moved out of the loop (along with n_fields
).
uint first_ind = sfpt->req() - sfpt->jvms()->scloff(); | ||
Node* sobj = new SafePointScalarObjectNode(gvn.type(res)->isa_oopptr(), | ||
#ifdef ASSERT | ||
call->isa_Allocate(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is always NULL
since call
can't be an Allocate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my suggestion to pass call
node as allocation so that we could trace back for what node SafePointScalarObject was created because you may have several Box objects for which we create SafePointScalarObject nodes.
I think we can change argument (and field type) to CallNode. And have assert in SafePointScalarObject constructor to check that it is either Allocate node or Boxing Call node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review. I will change the SafePointScalarObject constructor in next push.
/* | ||
* @test | ||
* @bug 8261137 | ||
* @requires vm.debug == true & vm.flavor == "server" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exclude Graal because it would not have output you expect. Instead of checking for "server" check for C2:
@requires vm.debug == true & vm.compiler2.enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I will do that ASAP.
@@ -0,0 +1,88 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should be in compiler/eliminateAutobox
directory. compiler/c2
is collection of old unsorted tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I will do that.
@@ -0,0 +1,113 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should be in compiler/eliminateAutobox directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Thank you for your review.
uint first_ind = sfpt->req() - sfpt->jvms()->scloff(); | ||
Node* sobj = new SafePointScalarObjectNode(gvn.type(res)->isa_oopptr(), | ||
#ifdef ASSERT | ||
call->isa_Allocate(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my suggestion to pass call
node as allocation so that we could trace back for what node SafePointScalarObject was created because you may have several Box objects for which we create SafePointScalarObject nodes.
I think we can change argument (and field type) to CallNode. And have assert in SafePointScalarObject constructor to check that it is either Allocate node or Boxing Call node.
src/hotspot/share/opto/callnode.cpp
Outdated
#ifdef ASSERT | ||
, _alloc(alloc) | ||
#endif | ||
{ | ||
#ifdef ASSERT | ||
if (alloc != nullptr) { | ||
assert(alloc->is_Allocate() || alloc->as_CallStaticJava()->is_boxing_method(), "unexpected alloc"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add dumping of alloc
before this assert to know it during failure without rerunning test:
if (!alloc->is_Allocate() && !alloc->as_CallStaticJava()->is_boxing_method()) {
alloc->dump();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my suggestion to pass call node as allocation so that we could trace back for what node SafePointScalarObject was created because you may have several Box objects for which we create SafePointScalarObject nodes.
The problem with that is alloc
quickly becomes stale since allocations/calls are removed shortly after scalarization takes place. The only usage of alloc()
is in PhaseMacroExpand::scalar_replacement
:
assert(scobj->alloc() == alloc, "sanity");
Regarding the assert, frankly speaking, it looks repetitive and verbose. I'd prefer to just see it folded into:
assert(alloc == NULL || alloc->is_Allocate() || alloc->as_CallStaticJava()->is_boxing_method(), "unexpected call node");
Or introduce a helper method to dump relevant info about the problematic node:
assert(alloc == NULL || alloc->is_Allocate() || alloc->as_CallStaticJava()->is_boxing_method(), "unexpected call node: %s", dump_node(alloc));
Also, alloc == NULL
case can be eliminated by avoiding passing NULL
in PhaseVector::scalarize_vbox_node()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Lets do as @iwanowww suggesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Lets do as @iwanowww suggesting.
Should we implement dump_node
or should we just use assert(alloc == NULL || alloc->is_Allocate() || alloc->as_CallStaticJava()->is_boxing_method(), "unexpected call node");
without dumping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating node's dump string in separate method is complicated.
We can just use false
in assert:
#ifdef ASSERT
if (alloc != nullptr && !alloc->is_Allocate() && !alloc->as_CallStaticJava()->is_boxing_method()) {
alloc->dump();
assert(false, "unexpected call node for scalar replacement");
}
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see your latest changes before answering your question. You did exactly as I suggested. Good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good.
Thank you for your approval. ;-) |
/integrate |
@Wanghuang-Huawei |
@vnkozlov Can you do me a favor to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too.
I will wait results of testing Vladimir I. is running. |
Test results (hs-tier1 - hs-tier4) are clean. /sponsor |
@iwanowww @Wanghuang-Huawei Since your change was applied there have been 217 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit eab8455. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
JDK-8075052 has removed useless autobox. However, in some cases, the box is still saved. For instance:
Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
The uncommon_trap is generated by the optimized "if", because its condition is always true.
We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
and deleting the use of box:
There is no additional fail/error(s) of jtreg after this patch.
I adjust my codes and add a new benchmark
aarch64:
base line:
opt:
x86:
base line:
opt:
Progress
Issue
Reviewers
Contributors
<wuyan34@huawei.com>
<aijiaming1@huawei.com>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2401/head:pull/2401
$ git checkout pull/2401
Update a local copy of the PR:
$ git checkout pull/2401
$ git pull https://git.openjdk.java.net/jdk pull/2401/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2401
View PR using the GUI difftool:
$ git pr show -t 2401
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2401.diff