jdk
jdk copied to clipboard
openjdk
8334231: Optimize MethodData layout
Hi all,
This PR is a part of https://bugs.openjdk.org/browse/JDK-8334227 to optimize Hotspot C++ class layouts, this one is for the layout of MethodData. Here is the original layout from pahole:
class MethodData : public Metadata {
public:
/* class Metadata <ancestor>; */ /* 0 0 */
/* XXX 8 bytes hole, try to pack */
class Method * _method; /* 8 8 */
int _size; /* 16 4 */
int _hint_di; /* 20 4 */
class Mutex _extra_data_lock; /* 24 104 */
/* --- cacheline 2 boundary (128 bytes) --- */
class CompilerCounters _compiler_counters; /* 128 80 */
/* --- cacheline 3 boundary (192 bytes) was 16 bytes ago --- */
intx _eflags; /* 208 8 */
intx _arg_local; /* 216 8 */
intx _arg_stack; /* 224 8 */
intx _arg_returned; /* 232 8 */
int _creation_mileage; /* 240 4 */
class InvocationCounter _invocation_counter; /* 244 4 */
class InvocationCounter _backedge_counter; /* 248 4 */
int _invocation_counter_start; /* 252 4 */
/* --- cacheline 4 boundary (256 bytes) --- */
int _backedge_counter_start; /* 256 4 */
uint _tenure_traps; /* 260 4 */
int _invoke_mask; /* 264 4 */
int _backedge_mask; /* 268 4 */
short int _num_loops; /* 272 2 */
short int _num_blocks; /* 274 2 */
enum WouldProfile _would_profile; /* 276 4 */
int _jvmci_ir_size; /* 280 4 */
/* XXX 4 bytes hole, try to pack */
class FailedSpeculation * _failed_speculations; /* 288 8 */
int _data_size; /* 296 4 */
int _parameters_type_data_di; /* 300 4 */
int _exception_handler_data_di; /* 304 4 */
/* XXX 4 bytes hole, try to pack */
intptr_t _data[1]; /* 312 8 */
/* size: 320, cachelines: 5, members: 27 */
/* sum members: 304, holes: 3, sum holes: 16 */
};
There are 3 holes in the layout, the 1st 8-byte hole seems related to the ancestor Metadata which actually has 8-byte size, we may not be able to do anything to optimize:
class Metadata : public MetaspaceObj {
public:
/* class MetaspaceObj <ancestor>; */ /* 0 0 */
/* XXX last struct has 1 byte of padding */
int ()(void) * * _vptr.Metadata; /* 0 8 */
/* size: 8, cachelines: 1, members: 2 */
/* paddings: 1, sum paddings: 1 */
/* last cacheline: 8 bytes */
};
The two 4-byte holes should be easy to fix, we can simply swap the position of _jvmci_ir_size and _failed_speculations for better alignment. Here is the new layout after the change:
class MethodData : public Metadata {
public:
/* class Metadata <ancestor>; */ /* 0 0 */
/* XXX 8 bytes hole, try to pack */
class Method * _method; /* 8 8 */
int _size; /* 16 4 */
int _hint_di; /* 20 4 */
class Mutex _extra_data_lock; /* 24 104 */
/* --- cacheline 2 boundary (128 bytes) --- */
class CompilerCounters _compiler_counters; /* 128 80 */
/* --- cacheline 3 boundary (192 bytes) was 16 bytes ago --- */
intx _eflags; /* 208 8 */
intx _arg_local; /* 216 8 */
intx _arg_stack; /* 224 8 */
intx _arg_returned; /* 232 8 */
int _creation_mileage; /* 240 4 */
class InvocationCounter _invocation_counter; /* 244 4 */
class InvocationCounter _backedge_counter; /* 248 4 */
int _invocation_counter_start; /* 252 4 */
/* --- cacheline 4 boundary (256 bytes) --- */
int _backedge_counter_start; /* 256 4 */
uint _tenure_traps; /* 260 4 */
int _invoke_mask; /* 264 4 */
int _backedge_mask; /* 268 4 */
short int _num_loops; /* 272 2 */
short int _num_blocks; /* 274 2 */
enum WouldProfile _would_profile; /* 276 4 */
class FailedSpeculation * _failed_speculations; /* 280 8 */
int _jvmci_ir_size; /* 288 4 */
int _data_size; /* 292 4 */
int _parameters_type_data_di; /* 296 4 */
int _exception_handler_data_di; /* 300 4 */
intptr_t _data[1]; /* 304 8 */
/* size: 312, cachelines: 5, members: 27 */
/* sum members: 304, holes: 1, sum holes: 8 */
/* last cacheline: 56 bytes */
};
The two 4-byte holes are removed, saving 8 bytes.
Also removed unnecessary private: mark.
Additional test:
- [x] CONF=linux-x86_64-server-fastdebug CONF_CHECK=ignore make clean test TEST=tier2
Best, Xiaolong.
Progress
- [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8334231: Optimize MethodData layout (Sub-task - P4)
Reviewers
- David Holmes (@dholmes-ora - Reviewer)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20019/head:pull/20019
$ git checkout pull/20019
Update a local copy of the PR:
$ git checkout pull/20019
$ git pull https://git.openjdk.org/jdk.git pull/20019/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20019
View PR using the GUI difftool:
$ git pr show -t 20019
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20019.diff
Webrev
:wave: Welcome back xpeng! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@pengxiaolong 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:
8334231: Optimize MethodData layout
Reviewed-by: dholmes, chagedorn, shade
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 78 new commits pushed to the master branch:
- 540188fdebd089d4145eca18c0f95bf338cbcefc: 8334445: Parallel: Decouple maximum compaction from SoftReference clearing
- 3cce31ad8877ec62429981871bcb0067770f9ccb: 8335643: serviceability/dcmd/vm tests fail for ZGC after JDK-8322475
- 55fd1ed228ea3c42aaf92579e5dcb818fe14351d: 8333890: Fatal error in auto-vectorizer with float16 kernel.
- 02956ab6e161ca8556a73f328f79bcbfba997cbc: 8332163: C2 SuperWord: refactor PacksetGraph and SuperWord::output into VTransformGraph
- 3f37c5718d676b7001e6a084aed3ba645745a144: 8335806: RISC-V: Corrected typos Bizarrely
- 6f7f0f1de05fdc0f6a88ccd90b806e8a5c5074ef: 8333884: MemorySegment::reinterpret removes read-only property
- b83766e59063a41ea8801ac9e7c15dce67727c62: 8335632: jdk/jfr/api/consumer/streaming/TestJVMExit.java failed with "Process [...] is no longer alive"
- 7efe16038e5df9894a265ea1214068060f595c4e: 8335730: JFR: Clean up jdk.jfr
- ff49f677ee5017019c90823bc412ceb90068ffbd: 8335775: Remove extraneous 's' in comment of rawmonitor.cpp test file
- 194425d7875ef42fce52516ed59c81ee97720399: 8335645: j.u.Formatter#trailingZeros improved with String repeat
- ... and 68 more: https://git.openjdk.org/jdk/compare/d9bcf061450ebfb7fe02b5a50c855db1d9178e5d...master
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.
As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @chhagedorn, @shipilev) but any other Committer may sponsor as well.
➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@pengxiaolong The following label will be automatically applied to this pull request:
hotspot
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.
@pengxiaolong Your change (at version 12e7aaf5c0a4ed529337d773e78a35a893f6cb44) is now ready to be sponsored by a Committer.
I don't think these "Optimize XXX layouts" should be marked as trivial, and I worry that we overuse the trivial rule. It circumvents the second reviewer as well as the 24hr rule, which both are necessary safeties. Especially in the wake of the xz fiasco.
"Trivial" is usually reserved for either changes that need very quick reaction (e.g. reasonably simple build errors that require immediate fixing because everyone's CI is standing still) or things that are painfully obvious in being trivial, e.g. comment changes. Memory layout changes are neither urgent nor really trivial enough IMHO.
That's a fair point. It's sometimes tricky to find the boundary between trivial and non-trivial. Here I thought about it for a bit but it looked trivial (but I understand that you could think about it differently) and I was the second reviewer. Nevertheless, as you pointed out, I agree that we should probably restrict the use of this rule to the really urgent issues (most are not). My default is usually to just wait 24h for normal issues even for changes that are marked as trivial just to give everyone a chance to have a look. Sometimes, you still get some valuable feedback that you could have missed otherwise when integrating early.
I consider moving declarations around trivial; as functionally there is no impact. There could be a performance impact but it is very unlikely that anyone would know for certain during a code review (unless we violate a comment saying that things needs to be spaced out for caching). But I agree there is also no urgency.
Thanks @tstuefe @chhagedorn @dholmes-ora for the reviews and discussion about the "trivial" topic, I agree that this may not be trivial and I'll be cautious when declare PR is trivial. Honestly I don't know if there are explicit rules for trivial/non-trivial, I felt it was very simple change swapping the locations of two fields and should be qualified as trivial(subjective judgement). I'm ok to remove the declaration about trivial. For the PR itself, there are already two reviewer approvals, and we have probably got enough eyes on it, if you don't have other concerns, I'll integrate it next week @tstuefe
Going to push as commit c5a668bb653feb3408a9efa3274ceabf9f01a2c7.
Since your change was applied there have been 78 commits pushed to the master branch:
- 540188fdebd089d4145eca18c0f95bf338cbcefc: 8334445: Parallel: Decouple maximum compaction from SoftReference clearing
- 3cce31ad8877ec62429981871bcb0067770f9ccb: 8335643: serviceability/dcmd/vm tests fail for ZGC after JDK-8322475
- 55fd1ed228ea3c42aaf92579e5dcb818fe14351d: 8333890: Fatal error in auto-vectorizer with float16 kernel.
- 02956ab6e161ca8556a73f328f79bcbfba997cbc: 8332163: C2 SuperWord: refactor PacksetGraph and SuperWord::output into VTransformGraph
- 3f37c5718d676b7001e6a084aed3ba645745a144: 8335806: RISC-V: Corrected typos Bizarrely
- 6f7f0f1de05fdc0f6a88ccd90b806e8a5c5074ef: 8333884: MemorySegment::reinterpret removes read-only property
- b83766e59063a41ea8801ac9e7c15dce67727c62: 8335632: jdk/jfr/api/consumer/streaming/TestJVMExit.java failed with "Process [...] is no longer alive"
- 7efe16038e5df9894a265ea1214068060f595c4e: 8335730: JFR: Clean up jdk.jfr
- ff49f677ee5017019c90823bc412ceb90068ffbd: 8335775: Remove extraneous 's' in comment of rawmonitor.cpp test file
- 194425d7875ef42fce52516ed59c81ee97720399: 8335645: j.u.Formatter#trailingZeros improved with String repeat
- ... and 68 more: https://git.openjdk.org/jdk/compare/d9bcf061450ebfb7fe02b5a50c855db1d9178e5d...master
Your commit was automatically rebased without conflicts.
@shipilev @pengxiaolong Pushed as commit c5a668bb653feb3408a9efa3274ceabf9f01a2c7.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.