Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AjaxRequestHandler: Keep listeners in a LinkedHashSet instead of LinkedList for performance reason #438

Merged
merged 1 commit into from
Jul 6, 2020

Conversation

ursjoss
Copy link
Contributor

@ursjoss ursjoss commented Jul 5, 2020

While migrating my project (see PR there) from the mocking library mockito to mock, I ran into weird performance issues. The build time jumped from roughly 20 minutes to eventually more than 4 hours. I took some effort improving various things in my own code (less mocking, more memory etc.) but did not bring it down considerably until I addressed a bottleneck I found in wicket.

AjaxRequestHandler maintains a collection of listeners in a LinkedList. When client code adds a new listener calling method addListener, it uses the collection's method contains to assert we don't add a listener to the collection twice. This doesn't scale well. For some reason, my setup with mockk seems to add many more listeners than what my code did with mockito, so I ran into this issue with continuously slower test executions.

I managed to temporarily resolve the problem in my project by plugging in my own copy of AjaxRequestHandler where I swapped the collection type from LinkedList to LinkedHashSet (calling setAjaxRequestTargetProvider in my TestApplication). With this approach the total build time went down again to 20-30 minutes.

This PR serves the purpose of starting a discussion about whether this change would be feasible to implement in wicket directly. To me, it is not apparent why a LinkedList had been chosen initially. The LinkedHashSet performs much better with contains and still preserves insertion order. If that latter should turn out to be not relevant, we could even consider using a HashSet instead of a linked HashSet, but I presume insertion order is important.

A separate topic on my side will be to figure out why my setup with mockito seemed to work well while migrating to mockk surfaced this issue.

@martin-g martin-g merged commit 5c63eb5 into apache:master Jul 6, 2020
martin-g pushed a commit that referenced this pull request Jul 6, 2020
…tead of LinkedList for performance reason (#438)

(cherry picked from commit 5c63eb5)
martin-g pushed a commit that referenced this pull request Jul 6, 2020
…tead of LinkedList for performance reason (#438)

(cherry picked from commit 5c63eb5)
@martin-g
Copy link
Member

martin-g commented Jul 6, 2020

Thank you, @ursjoss !

@ursjoss ursjoss deleted the test_performance branch July 6, 2020 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants