RFR: 8334231: Optimize MethodData layout

[Xiaolong Peng]

On Thu, 4 Jul 2024 12:41:15 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> 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, cachelin...
>
> 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.

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 is 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

-------------

PR Comment: https://git.openjdk.org/jdk/pull/20019#issuecomment-2213124652