Uploaded image for project: 'Wicket'
  1. Wicket
  2. WICKET-6800

Use LinkedHashSet instead of LinkedList for AjaxRequestHandler#listeners

    XMLWordPrintableJSON

Details

    • Improvement
    • Status: Resolved
    • Minor
    • Resolution: Fixed
    • 9.0.0-M5, 8.8.0
    • 7.17.0, 9.0.0, 8.9.0
    • wicket
    • None

    Description

      https://github.com/apache/wicket/pull/438

      Urs Joss:
      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.

      Attachments

        Activity

          People

            mgrigorov Martin Tzvetanov Grigorov
            mgrigorov Martin Tzvetanov Grigorov
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: