review: perf: avoid collection copy in CtPackageImpl #4848
Conversation
SirYwell
changed the title
| @@ -242,7 +242,12 @@ public boolean hasPackageInfo() { | |||
|
|
|||
| @Override | |||
| public boolean isEmpty() { | |||
| return getPackages().isEmpty() && getTypes().isEmpty(); | |||
| return getPackages().isEmpty() && !hasTypes(); | |||
There was a problem hiding this comment.
Doesn't getPackages() have precisely the same problem (allocates a new LinkedHashSet on each invocation)? Perhaps kill two birds with one stone in this PR?
There was a problem hiding this comment.
Yes, getPackages() has the same problem, but as it didn't really show up in my profiles (as it's called far less), I didn't include an extra method for the empty check in this PR. If you want, I can do so though.
There was a problem hiding this comment.
It's probably not a performance problem because the amount of packages is typically far fewer than the amount of types in any given package.
I think from an API standpoint it makes sense to have both. Since there are methods getTypes() and getPackages(), I probably would expect a hasTypes() to be accompanied by a hasPackages(). What do you think?
| /** | ||
| * @return true if the package contains any types. | ||
| * @see #getTypes() | ||
| */ |
There was a problem hiding this comment.
Now that I think about it, we should probably add a requirement here that this is O(1). That's really the reason for it to exist in the first place.
On that note, we maybe should add notes to getTypes() and getPackages() that they may be linear in the amount of types/packages, and one should use isEmpty(), hasPackages() (if we add it) or hasTypes() to check for "emptiness".
|
I added some runtime behavior documentation and the |
I saw that when running this test
spoon/src/test/java/spoon/test/architecture/SpoonArchitectureEnforcerTest.java
Line 157 in 0fcb288
most of the time is spent on
CtMethodImpl#getTopDefintions(). I did some profiling and encountered this:That's an immense amount of time of copying elements just to check if the set is empty. As a solution, I propose to introduce a
CtPackage#hasTypes()method. This way, we can ask the ElementNameMap directly instead of doing costly copying.In my first measurements, this reduced the runtime of the mentioned test from ~20s to ~16s.
(I rebased after that, and the runtime of the test on the current master takes >1m on my machine. With this patch, it is still faster, but I have a second PR ready to get back to the ~16s)