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
8260012: Reduce inclusion of collectedHeap.hpp and heapInspection.hpp #2347
8260012: Reduce inclusion of collectedHeap.hpp and heapInspection.hpp #2347
Conversation
|
/label remove serviceability |
@iklam |
/label add gc |
@iklam The label
|
/label add hotspot-gc |
@iklam |
Webrevs
|
Looks good. A few things that you might want to consider, but I'm also fine with the patch as it is.
#include "memory/memRegion.hpp" | ||
#include "oops/oopsHierarchy.hpp" | ||
#include "runtime/thread.hpp" |
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.
If we want to, this could be changed to a forward declaration if we removed the default value (Thread* thread = Thread::current()) of the constructors. Not needed for this RFE though.
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.
memAllocator.hpp is not included very often (only 65 out of ~1000 .o files), so I decided to leave it as is.
src/hotspot/cpu/arm/frame_arm.cpp
Outdated
@@ -515,7 +515,7 @@ BasicType frame::interpreter_frame_result(oop* oop_result, jvalue* value_result) | |||
} else { | |||
obj = *(oop*)res_addr; | |||
} | |||
assert(obj == NULL || Universe::heap()->is_in(obj), "sanity check"); | |||
assert(obj == NULL || Universe::is_in_heap(obj), "sanity check"); |
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.
Could have been changed to is_in_heap_or_null.
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.
Fixed
src/hotspot/cpu/ppc/frame_ppc.cpp
Outdated
@@ -305,7 +305,7 @@ BasicType frame::interpreter_frame_result(oop* oop_result, jvalue* value_result) | |||
case T_OBJECT: | |||
case T_ARRAY: { | |||
oop obj = *(oop*)tos_addr; | |||
assert(obj == NULL || Universe::heap()->is_in(obj), "sanity check"); | |||
assert(obj == NULL || Universe::is_in_heap(obj), "sanity check"); |
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.
Could have been changed to is_in_heap_or_null.
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.
Fixed. I also change other frame_.cpp files to use is_in_heap_or_null.
src/hotspot/cpu/s390/frame_s390.cpp
Outdated
@@ -318,7 +318,7 @@ BasicType frame::interpreter_frame_result(oop* oop_result, jvalue* value_result) | |||
case T_OBJECT: | |||
case T_ARRAY: { | |||
oop obj = *(oop*)tos_addr; | |||
assert(obj == NULL || Universe::heap()->is_in(obj), "sanity check"); | |||
assert(obj == NULL || Universe::is_in_heap(obj), "sanity check"); |
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.
Could have been changed to is_in_heap_or_null.
@iklam This change now passes all automated pre-integration checks. 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 no new commits pushed to the
|
Checked a few includes for missing ones; obviously they are included transitively so add as you see fit.
#include "memory/memRegion.hpp" | ||
#include "oops/oopsHierarchy.hpp" | ||
#include "runtime/thread.hpp" |
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.
utilities/globalDefinitions.hpp
for HeapWord
is missing.
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.
Fixed.
@@ -25,7 +25,6 @@ | |||
#ifndef SHARE_OOPS_COMPRESSEDOOPS_INLINE_HPP | |||
#define SHARE_OOPS_COMPRESSEDOOPS_INLINE_HPP | |||
|
|||
#include "gc/shared/collectedHeap.hpp" |
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.
utilities/globalDefinitions.hpp
for *PTR_FORMAT
and others is missing.
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.
Fixed.
@@ -25,7 +25,6 @@ | |||
#ifndef SHARE_OOPS_OOP_INLINE_HPP | |||
#define SHARE_OOPS_OOP_INLINE_HPP | |||
|
|||
#include "gc/shared/collectedHeap.hpp" |
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.
utilities/globalDefinitions.hpp
for HeapWord
is missing.
globals.hpp
for some globals.
oopsHierarchy.hpp
for narrowKlass
utilties/debug.hpp
for assert
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.
Fixed. Thanks for the review.
Still good. |
…reduce-inclue-collectedHeap-heapInspection-hpp
/integrate |
collectedHeap.hpp is included by 477 out of 1000 .o files in HotSpot. This file in turn includes many other complex header files.
In many cases, an object file only directly includes this file via:
Universe::heap()->is_in()
).By refactoring the above 3 files, we can reduce the .o files that include collectedHeap.hpp to 242.
This RFE also removes the unnecessary inclusion of heapInspection.hpp from collectedHeap.hpp.
Build time of HotSpot is reduced for about 1%.
Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2347/head:pull/2347
$ git checkout pull/2347