It's merged into 10.x, 9.x and 8.x Martijn
On Sun, Sep 4, 2022 at 12:17 AM Andrea Del Bene <an.delb...@gmail.com> wrote: > > On 03/09/22 20:46, Martijn Dashorst wrote: > > 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 > > Ok! Thank you! > > > > > > > > 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
