I've added PRs for wicket 8.x, 9.x and master. I assume we won't be releasing a 7.x anytime soon, so this is just one more benefit for upgrading to a newer version IMO.
Even though this came up as a blocking hotspot, it did not show up as a performance hotspot, so it will benefit all wicket applications, but it is not detrimental. If there are no objections I intend to merge these changes somewhere on monday. Martijn On Fri, Sep 2, 2022 at 5:43 PM Martijn Dashorst <martijn.dasho...@gmail.com> wrote: > I've created this ticket for this: > > https://issues.apache.org/jira/browse/WICKET-7002 > > And a PR: https://github.com/apache/wicket/pull/532 > > Martijn > > > On Fri, Sep 2, 2022 at 2:37 PM Martijn Dashorst < > martijn.dasho...@gmail.com> wrote: > >> Well, >> >> I think that the metadata storage in Application tries to mimic a >> key-value store, but does so in an esoteric way that requires >> synchronization, which is unnecessary when we just use a ConcurrentHashMap >> for the datastructure that is especially designed for this use case. >> Changing the array to a ConcurrentHashMap will not increase the memory >> footprint in any meaningful way, but will remove the need for >> synchronization and change the algorithm for looking up a value by key from >> O(n) to O(1) ( >> https://stackoverflow.com/questions/559839/big-o-summary-for-java-collections-framework-implementations >> ). >> >> The change is completely encapsulated for external users (unless they >> play with the internals using reflection) so this can also be backported to >> Wicket 9, 8 and 7, up til Wicket 1.5 if we so desire (we don't but you get >> my drift). >> >> I also think it is a bad idea to return the datastructure from the >> setter. This would have made this optimization impossible to do in a >> backwards compatible way. >> >> Martijn >> >> >> On Fri, Sep 2, 2022 at 11:59 AM Andrea Del Bene <an.delb...@gmail.com> >> wrote: >> >>> It's of course a delicate topic and it needs a full investigation, but >>> looking at the code of both setMetaData and getMetaData I think we could >>> reduce the synchronization overloading by making Application#metadata >>> volatile and removing synchronized from getMetaData. In addition I think >>> setMetaData should always return a new copy of Application#metadata to >>> ensure a proper synchronization among threads, but I'm not 100% sure >>> about >>> this. >>> >>> Just some 2 cents... >>> >>> On Fri, Sep 2, 2022 at 8:46 AM Martijn Dashorst < >>> martijn.dasho...@gmail.com> >>> wrote: >>> >>> > Heya! >>> > >>> > In a performance dump for a production issue I was looking through the >>> > monitors that were in use during request processing and I noticed that >>> a >>> > lot of request threads were blocking on Application.getMetaData. >>> > >>> > This method is synchronized because it uses an array of metadata and >>> goes >>> > through that array in search of the key. >>> > >>> > I'm no rocket scientist, but shouldn't that metadata field just be a >>> > ConcurrentHashMap so we can remove the blocking synchronization on the >>> > application object? >>> > >>> > Martijn >>> > >>> > <http://wicketinaction.com> >>> > >>> >>> >>> -- >>> Andrea Del Bene. >>> Apache Wicket committer. >>> >> >> >> -- >> Become a Wicket expert, learn from the best: http://wicketinaction.com >> > > > -- > Become a Wicket expert, learn from the best: http://wicketinaction.com > -- Become a Wicket expert, learn from the best: http://wicketinaction.com
