[GitHub] [wicket] ursjoss opened a new pull request #438: AjaxRequestHandler: Keep listeners in a LinkedHashSet instead of LinkedList for performance reason

[GitBox <gi...@apache.org>]

ursjoss opened a new pull request #438:
URL: https://github.com/apache/wicket/pull/438


   While migrating my project (see [PR](https://github.com/ursjoss/scipamato/pull/177) there) from the mocking library [mockito](https://site.mockito.org/) to [mock](https://mockk.io/), 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](https://github.com/ursjoss/scipamato/blob/master/core/core-web/src/test/kotlin/ch/difty/scipamato/TestAjaxRequestHandler.kt#L101) where I swapped the collection type from LinkedList to LinkedHashSet (calling `setAjaxRequestTargetProvider` in my [TestApplication](https://github.com/ursjoss/scipamato/blob/master/core/core-web/src/test/kotlin/ch/difty/scipamato/TestApplication.kt#L29)). 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.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [wicket] martin-g merged pull request #438: AjaxRequestHandler: Keep listeners in a LinkedHashSet instead of LinkedList for performance reason

[GitBox <gi...@apache.org>]

martin-g merged pull request #438:
URL: https://github.com/apache/wicket/pull/438


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [wicket] martin-g commented on pull request #438: AjaxRequestHandler: Keep listeners in a LinkedHashSet instead of LinkedList for performance reason

[GitBox <gi...@apache.org>]

martin-g commented on pull request #438:
URL: https://github.com/apache/wicket/pull/438#issuecomment-654092789


   Thank you, @ursjoss !


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org