Skip to content
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

[Backport] 8333334: C2: Make result of Node::dominates more precise to enhance scalar replacement #105

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

MaxXSoft
Copy link
Collaborator

@MaxXSoft MaxXSoft commented Sep 5, 2024

[Backport] 8333334: C2: Make result of Node::dominates more precise to enhance scalar replacement

Summary: This patch changes the algorithm of Node::dominates to make the result more precise, allows the iterators of ConcurrentHashMap to be scalar replaced, and makes the iteration ~30% faster.

Testing: CI testing, IR test

Reviewers: kuaiwei, JoshuaZhuwj

Issue: #104

… to enhance scalar replacement

Summary: This patch changes the algorithm of `Node::dominates` to make the result more precise, allows the iterators of `ConcurrentHashMap` to be scalar replaced, and makes the iteration ~30% faster.

Testing: CI testing, IR test

Reviewers: kuaiwei, JoshuaZhuwj

Issue: dragonwell-project#104
@MaxXSoft
Copy link
Collaborator Author

MaxXSoft commented Sep 5, 2024

This backport is not clean due to missing backport of JDK-8287061 (openjdk/jdk#12897), which is too complex to backport.

Before applying this backport:

Benchmark                                  (nkeys)  Mode  Cnt        Score       Error  Units
Maps.testConcurrentHashMapIterators          10000  avgt   15   390877.321 ±  3723.284  ns/op

After:

Benchmark                                  (nkeys)  Mode  Cnt        Score       Error  Units
Maps.testConcurrentHashMapIterators          10000  avgt   15   330291.004 ± 14000.149  ns/op

Copy link
Collaborator

@JoshuaZhuwj JoshuaZhuwj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

有个建议:因为是patch backport到dw21,所以我们需要考虑将来“后续patch backport”,以及“自动和jdk21同步”是否会产生冲突,所以理想情况是,尽量减少将来backport & auto merge可能产生冲突的可能性。所以可以考虑删除引入“DomResult”,只引入"将Load node加入igvn的worklist“这一个单独的change。但是因为相关的函数本身在后续可能产生冲突的可能性很小,所以这个backport,没有问题。approve. Thanks!

Copy link
Collaborator

@kuaiwei kuaiwei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

使用all_controls_dominate和maybe_all_controls_dominate的场合有规则吗

src/hotspot/share/opto/memnode.cpp Show resolved Hide resolved
@MaxXSoft
Copy link
Collaborator Author

@kuaiwei

使用all_controls_dominate和maybe_all_controls_dominate的场合有规则吗

all_controls_dominate 和添加这个 patch 之前原有的 all_controls_dominate 含义一致;maybe_all_controls_dominate 在遇到死代码时会返回更精确的结果,其余和 all_controls_dominate 一致。

maybe_all_controls_dominate 只在 LoadNode::split_through_phi 中使用,也就是这个 patch 的核心实现:如果在判断 dom 的时候遇到死代码,就等死代码被删掉之后重新判断一次。

其余所有情况都使用 all_controls_dominate

Copy link
Collaborator

@kuaiwei kuaiwei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MaxXSoft
Copy link
Collaborator Author

Thanks for the review!

@MaxXSoft MaxXSoft merged commit 892963d into dragonwell-project:master Sep 20, 2024
91 of 92 checks passed
@MaxXSoft MaxXSoft deleted the backport-8333334 branch September 20, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants