On Tue, 22 Nov 2022 14:48:11 GMT, Afshin Zafari <d...@openjdk.org> wrote:
> test of tier1-5 passed.
Hi Afshin,
The general conversion approach seems okay, though it is hard to see exactly
how the new table is used compared to the old table i.e. in relation to the
role of `JvmtiTagMapEntry` now that it is not needed for the actual table. ??
A few queries below and a lot of nits to fix up (sorry).
Thanks.
src/hotspot/share/prims/jvmtiTagMap.cpp line 173:
> 171: //
> 172: static inline jlong tag_for(JvmtiTagMap* tag_map, oop o) {
> 173: return tag_map->hashmap()->find(o); } // A CallbackWrapper is a
> support class for querying and tagging an object // around a callback to a
> profiler. The constructor does pre-callback // work to get the tag value,
> klass tag value, ... and the destructor // does the post-callback work of
> tagging or untagging the object. // // {
The formatting of this line is messed up.
src/hotspot/share/prims/jvmtiTagMap.cpp line 215:
> 213:
> 214: // get object tag
> 215:
The comment and action are now out-of-order.
src/hotspot/share/prims/jvmtiTagMap.cpp line 254:
> 252: if (obj_tag != current_tag ) {
> 253: hashmap->remove(o);
> 254: hashmap->add(o, obj_tag);
This change is not atomic - is that a problem? The concurrency aspects of using
this map are not clear.
src/hotspot/share/prims/jvmtiTagMap.cpp line 303:
> 301: _referrer_obj_tag = _referrer_hashmap->find(_referrer);
> 302:
> 303: // get object tag
Action the comment and action are now out-of-order.
src/hotspot/share/prims/jvmtiTagMap.cpp line 313:
> 311: ~TwoOopCallbackWrapper() {
> 312: if (!is_reference_to_self()){
> 313:
Unnecessary blank line added.
src/hotspot/share/prims/jvmtiTagMap.cpp line 345:
> 343:
> 344: //JvmtiTagMapEntry entry ;
> 345: //_hashmap->add_update_remove(&entry, o, tag);
What are these comments for?
src/hotspot/share/prims/jvmtiTagMap.cpp line 365:
> 363: } else {
> 364: hashmap->remove(o);
> 365: hashmap->add(o,tag);
Nit: need space after comma
src/hotspot/share/prims/jvmtiTagMap.cpp line 1261:
> 1259: // and record the reference and tag value.
> 1260: //
> 1261: bool do_entry(JvmtiTagMapEntry & key , jlong & value ) {
Nit: no space before &
src/hotspot/share/prims/jvmtiTagMap.cpp line 1262:
> 1260: //
> 1261: bool do_entry(JvmtiTagMapEntry & key , jlong & value ) {
> 1262: for (int i=0; i<_tag_count; i++) {
Pre-existing: need spaces around binary operators = and <
src/hotspot/share/prims/jvmtiTagMapTable.cpp line 77:
> 75:
> 76: void JvmtiTagMapTable::clear() {
> 77: struct RemoveAll{
Nit: space before {
Stylistically I'm not sure we define local structs like this when defining a
"closure" object. ??
src/hotspot/share/prims/jvmtiTagMapTable.cpp line 89:
> 87:
> 88: JvmtiTagMapTable::~JvmtiTagMapTable() {
> 89: clear();
Is this the only use of `clear`? If so we can just inline its code here and
remove it from the API.
src/hotspot/share/prims/jvmtiTagMapTable.cpp line 95:
> 93: //if (obj->fast_no_hash_check()) {
> 94: // return 0;
> 95: //} else {
What are these comments?
src/hotspot/share/prims/jvmtiTagMapTable.cpp line 102:
> 100: }
> 101:
> 102: bool JvmtiTagMapTable::add(oop obj, jlong tag) {
I'm not seeing that a return value has any use here when it is always expected
to be true.
src/hotspot/share/prims/jvmtiTagMapTable.cpp line 123:
> 121:
> 122: void JvmtiTagMapTable::remove_dead_entries(GrowableArray<jlong>*
> objects) {
> 123: struct IsDead{
Nit: space before {
Same query about using a local struct for this.
src/hotspot/share/prims/jvmtiTagMapTable.cpp line 128:
> 126: bool do_entry(JvmtiTagMapEntry const & entry, jlong tag){
> 127: if ( entry.object_no_keepalive() == NULL){
> 128: if(_objects!=NULL){
Nit: need space before {
Nit: need space after if
Nit: no space after (
src/hotspot/share/prims/jvmtiTagMapTable.hpp line 69:
> 67: JvmtiTagMapEntry::get_hash,
> 68: JvmtiTagMapEntry::equals
> 69: > ResizableResourceHT ;
Nit: keep > on previous line
Nit: no space before ;
src/hotspot/share/prims/jvmtiTagMapTable.hpp line 79:
> 77:
> 78: void resize_if_needed();
> 79: ResizableResourceHT _rrht_table;
This can just be `_table` - no need for the `rrht` prefix.
src/hotspot/share/prims/jvmtiTagMapTable.hpp line 104:
> 102: class JvmtiTagMapEntryClosure {
> 103: public:
> 104: virtual bool do_entry(JvmtiTagMapEntry & key , jlong & value) = 0;
Nit: no space needed before &
-------------
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11288
