RFR: 8303150: DCmd framework unnecessarily creates a DCmd instance on registration [v3]
David Holmes
dholmes at openjdk.org
Thu Mar 16 06:26:59 UTC 2023
> 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:
Make get_parsed_num_arguments() ASSERT only code
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/12994/files
- new: https://git.openjdk.org/jdk/pull/12994/files/cfe345d8..fc46d253
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12994&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12994&range=01-02
Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/12994.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12994/head:pull/12994
PR: https://git.openjdk.org/jdk/pull/12994
More information about the hotspot-dev
mailing list