8371643: Remove ThreadLocalAllocBuffer::_reserve_for_allocation_prefetch #28240
Conversation
|
👋 Welcome back ayang! A progress list of the required criteria for merging this PR into |
|
@albertnetymk 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: 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 19 new commits pushed to the
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 |
openjdk
bot
changed the title
|
@albertnetymk 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. |
Webrevs
|
|
What happens if TLAB is at the end of heap page? Are you sure "prefetch" instructions on all OpenJDK platforms can touch unmapped memory? I see PPC can use |
|
Please ask all OpenJDK platforms supporters to test these changes. Note, when this code was introduced we did not have so many platforms. |
vnkozlov
left a comment
vnkozlov
left a comment
There was a problem hiding this comment.
You missed code in HS agent: src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VM.java
In every I searched online that all currently supported platforms don't fault on prefetching invalid addresses. |
|
@albertnetymk |
vnkozlov
left a comment
vnkozlov
left a comment
There was a problem hiding this comment.
I ran our testing (tier1-5) and there are no new failures. So I approve this change.
But you need second review. Preferable from other platforms supporters (RISC-V, PPC64, s390)
|
We have seen crashes on many platforms (including x64) while running |
I suspect the crash is caused by a preexisting issue that is exposed by this patch. In #if INCLUDE_CDS
if (CDSConfig::is_using_aot_linked_classes()) {
AOTLinkedClassBulkLoader::preload_classes(THREAD);
}
#endif
// Preload commonly used klasses
vmClassID scan = vmClassID::FIRST;
// first do Object, then String, Class
resolve_through(VM_CLASS_ID(Object_klass), scan, CHECK);
CollectedHeap::set_filler_object_klass(vmClasses::Object_klass());The filler-klass is not initialized when @iklam What do you think? /cc hotspot-gc |
|
@albertnetymk |
tschatzl
left a comment
tschatzl
left a comment
There was a problem hiding this comment.
Change looks good, but these AOT-related crashes should be fixed first.
I am working on a fix now. |
|
@albertnetymk I've pushed #28315. Please verify if it fixes the crash before integrating this PR. Thanks! |
The crashes are fixed. Thanks! |
|
This change looks incomplete to me. @vnkozlov: Shouldn't we remove |
|
This patch removes the reserved alignment that was required for
Will remove it in the next revision. |
|
At least PPC64 will need an update because your change breaks the following nodes: diff --git a/src/hotspot/cpu/ppc/ppc.ad b/src/hotspot/cpu/ppc/ppc.ad
index 7fcd096d2ad..c169d673aaf 100644
--- a/src/hotspot/cpu/ppc/ppc.ad
+++ b/src/hotspot/cpu/ppc/ppc.ad
@@ -6328,36 +6328,8 @@ instruct loadConD_Ex(regD dst, immD src) %{
// Prefetch instructions.
// Must be safe to execute with invalid address (cannot fault).
-// Special prefetch versions which use the dcbz instruction.
-instruct prefetch_alloc_zero(indirectMemory mem, iRegLsrc src) %{
- match(PrefetchAllocation (AddP mem src));
- predicate(AllocatePrefetchStyle == 3);
- ins_cost(MEMORY_REF_COST);
-
- format %{ "PREFETCH $mem, 2, $src \t// Prefetch write-many with zero" %}
- size(4);
- ins_encode %{
- __ dcbz($src$$Register, $mem$$base$$Register);
- %}
- ins_pipe(pipe_class_memory);
-%}
-
-instruct prefetch_alloc_zero_no_offset(indirectMemory mem) %{
- match(PrefetchAllocation mem);
- predicate(AllocatePrefetchStyle == 3);
- ins_cost(MEMORY_REF_COST);
-
- format %{ "PREFETCH $mem, 2 \t// Prefetch write-many with zero" %}
- size(4);
- ins_encode %{
- __ dcbz($mem$$base$$Register);
- %}
- ins_pipe(pipe_class_memory);
-%}
-
instruct prefetch_alloc(indirectMemory mem, iRegLsrc src) %{
match(PrefetchAllocation (AddP mem src));
- predicate(AllocatePrefetchStyle != 3);
ins_cost(MEMORY_REF_COST);
format %{ "PREFETCH $mem, 2, $src \t// Prefetch write-many" %}
@@ -6370,7 +6342,6 @@ instruct prefetch_alloc(indirectMemory mem, iRegLsrc src) %{
instruct prefetch_alloc_no_offset(indirectMemory mem) %{
match(PrefetchAllocation mem);
- predicate(AllocatePrefetchStyle != 3);
ins_cost(MEMORY_REF_COST);
format %{ "PREFETCH $mem, 2 \t// Prefetch write-many" %} |
TheRealMDoerr
left a comment
TheRealMDoerr
left a comment
There was a problem hiding this comment.
Thanks for the updates! make run-test TEST=test/hotspot/jtreg/compiler/c2 JTREG="VM_OPTIONS=-XX:AllocatePrefetchStyle=3" has passed on PPC64. I agree, the general AllocatePrefetchStyle==3 topic can be discussed separately.
|
Thanks for review. /integrate |
|
Going to push as commit 50a3049.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@albertnetymk Pushed as commit 50a3049. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
5 participants
Trivial removing obsoleted code for unsupported arch.
Test: tier1
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28240/head:pull/28240$ git checkout pull/28240Update a local copy of the PR:
$ git checkout pull/28240$ git pull https://git.openjdk.org/jdk.git pull/28240/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28240View PR using the GUI difftool:
$ git pr show -t 28240Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28240.diff
Using Webrev
Link to Webrev Comment