Home Original page

DepMgr hashCode handling by cstamas · Pull Request #749 · apache/maven-resolver

@cstamas

As graph is growing, same instances of HashMaps are repeatedly asked for hashCode, that in default implementation goes over all keys and values over and over again.

Changes:

  • fix missing entry in equals as @mbien reported
  • "invent" a special construct that fits this very use case: map is modified once, and copied if next level is about to modify it
  • the construct once asked for hashCode, memoizes it (the "read only" check never happens in this code, is just there to be foolproof)

@cstamas

@cstamas cstamas changed the title Collection hashCode DepMgr hashCode handling

Jun 12, 2025

@cstamas

gnodet

mbien

Comment on lines +97 to +104

if (hashCode == null) {
synchronized (delegate) {
if (hashCode == null) {
hashCode = delegate.hashCode();
}
}
}
return hashCode;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would probably use the simple "racy" version of this similar like it used to be:

public int hashCode() {
if (hashCode == 0) {
hashCode = Objects.hash(depth, managedVersions, managedScopes, managedOptionals, managedExclusions);
}
return hashCode;
}

this should be safe since:

  • doc requires managers to be stateless, I assume this means all maps are immutable
  • this will be likely only used from one thread
  • race condition would not be a big deal since all it would to is to introduce a chance that the value is computed more than once under high contention

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have other idea, will add done() method....

@cstamas

@cstamas cstamas marked this pull request as ready for review

June 12, 2025 20:32

mbien

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me, left one comment.

Comment on lines +68 to +70

public MMap<K, V> done() {
return new DoneMMap<>(delegate);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

    return this instanceof DoneMMap ? this : new DoneMMap<>(delegate);
}

otherwise this would keep copying the maps even if nothing changed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but done method is overridden in DoneMMap? I don't get it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait maybe I am wrong here - this likely can't happen since done() of the frozen instance is a no-op

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry!

@cstamas

@cstamas

@mbien

it will copy the map twice now though.

first:
MMap.copy(this.managedExclusions);

second later:
managedExclusions.done()

which will hit the same copy constructor again.

@cstamas

it will copy the map twice now though.

first: MMap.copy(this.managedExclusions);

second later: managedExclusions.done()

which will hit the same copy constructor again.

#750