On Fri, 27 Sep 2024 09:50:21 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
> I have only cosmetic comments. Do you know why these were statics to begin
> with?
AFAIU they were introduced to simplify access to currently active dumper (and
its writer - HeapDumper had a single writer that days) from callback functions
(static methods like `do_load_class` updated by this fix).
> src/hotspot/share/services/heapDumper.cpp line 1520:
>
>> 1518: private:
>> 1519: AbstractDumpWriter* _writer;
>> 1520: GrowableArray<Klass*>* _klass_map;
>
> While we are at it, turn these `const`?
the map is updated by the class, so it can't be `const`
> src/hotspot/share/services/heapDumper.cpp line 1526:
>
>> 1524: _klass_map->at_put_grow(serial_num, k);
>> 1525: }
>> 1526: public:
>
> Suggestion:
>
> void add_class_serial_number(Klass* k, int serial_num) {
> _klass_map->at_put_grow(serial_num, k);
> }
>
> public:
Fixed
> src/hotspot/share/services/heapDumper.cpp line 1532:
>
>> 1530: };
>> 1531:
>> 1532: void LoadedClassDumper::do_klass(Klass* k) {
>
> Any reason not to merge this definition straight in declaration?
Merged.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/21216#issuecomment-2379944243
PR Review Comment: https://git.openjdk.org/jdk/pull/21216#discussion_r1779080190
PR Review Comment: https://git.openjdk.org/jdk/pull/21216#discussion_r1779080374
PR Review Comment: https://git.openjdk.org/jdk/pull/21216#discussion_r1779081395
