RFR: 8303150: DCmd framework unnecessarily creates a DCmd instance on registration [v2]

[Thomas Stuefe]

On Wed, 15 Mar 2023 02:36:03 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> When DCmd factories are registered, the factory is passed the number of arguments taken by the DCmd - using a template method `get_num_arguments`. For DCmds that don't extend DCmdWithParser there has to be a static `num_arguments()` method in that class. For DCmds that do extend DCmdWithParser the logic instantiates an instance of the DCmd, extracts its parser and calls its `num_arguments` method which dynamically counts the number of defined arguments.
>> 
>> Creating an instance of each DCmd and dynamically counting arguments is inefficient and unnecessary, the number of arguments is statically known and easily expressed (in fact many of the JFR DCmds already statically define this). So we add the static `num_arguments()` method to each class that needs it and return the statically counted number of arguments. To ensure the static number and actual number don't get out-of-sync, we keep the original dynamic logic for use in debug builds to assert that the static and dynamic counts are the same. The assert will trigger during a debug build if something does get out of sync, for example if a new DCmd (extending DCmdWithParser) were added but didn't define the static `num_arguments()` method.
>> 
>> A number of DCmd classes were unnecessarily defining their own dynamic version of `num_arguments` and these are now removed.
>> 
>> In the template method I use `ENABLE_IF(std::is_convertible<T, DCmd>::value)` to check we only call this on DCmd classes. This may be unnecessary but it seemed consistent with the other template methods. Note that `std::is_base_of` only works for immediate super types.
>> 
>> Testing: tiers 1-4
>> 
>> Performance: in theory we should see some improvement in startup; in practice it is barely noticeable.
>> 
>> Thanks.
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Comment update - Kevin's feedback

Thanks a lot for removing that blemish, David! That has bugged me (and others) repeatedly.

Question inline, but looks good all in all.

src/hotspot/share/services/diagnosticFramework.hpp line 446:

> 444: 
> 445:   template <typename T, ENABLE_IF(std::is_base_of<DCmdWithParser, T>::value)>
> 446:   static int get_parsed_num_arguments() {

I don't understand why we need the dynamic allocation here. Should we not be able to call your new static versions of num_arguments for DCmdWithParser children, like we do for DCmd children above? And if so, could we then not unify those two functions?

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

PR: https://git.openjdk.org/jdk/pull/12994