You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by GitBox <gi...@apache.org> on 2022/04/19 01:48:37 UTC
[GitHub] [wicket] astange1 opened a new pull request, #512: List.of() creates an immutable List instance, without a backing Array...
astange1 opened a new pull request, #512:
URL: https://github.com/apache/wicket/pull/512
…List, saving one Object[] array creation and saving ~12 bytes for each component in the hierarchy. This ends up being a few KB for each of our pages, resulting in meaningful memory savings, a little less GC pressure and a little more memory fitting into the processor caches.
This is a minor change. I expect I will be sending along other small improvements like this one for the Wicket code base if there is an interest in receiving them. FYI We have been a Wicket user since ~2008.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
theigl commented on PR #512:
URL: https://github.com/apache/wicket/pull/512#issuecomment-1102970683
@astange1: There is some related code that might also benefit from using `List.of`. E.g.
https://github.com/apache/wicket/blob/c6d18e697b699723ca2cc3788e64b56f5929445e/wicket-core/src/main/java/org/apache/wicket/markup/head/JavaScriptContentHeaderItem.java#L75
And all other occurrences of `Arrays.asList` in `org.apache.wicket.markup.head`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a diff in pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #512:
URL: https://github.com/apache/wicket/pull/512#discussion_r854046015
##########
wicket-core/src/main/java/org/apache/wicket/Component.java:
##########
@@ -2669,7 +2669,7 @@ public void internalRenderHead(final HtmlHeaderContainer container)
if (response.wasRendered(behavior) == false)
Review Comment:
@martin-g: I pushed https://github.com/apache/wicket/pull/515 that fixes this and incorporates the change suggested in this PR.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
theigl commented on PR #512:
URL: https://github.com/apache/wicket/pull/512#issuecomment-1103877388
@astange1: I resolved this in #515. It is easier to backport if the fix and the improvement are done in a single PR/commit. Thanks a lot for the suggestion!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] astange1 commented on pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
astange1 commented on PR #512:
URL: https://github.com/apache/wicket/pull/512#issuecomment-1102993467
I would be happy to proceed and make those needed edits. I did just that one for two reasons:
1) it shows up as being a somewhat hot in some of my profiles.
2) I figured I would do that one to start. If that simple PR was accepted then I would gladly do more, and if not then I hadn't wasted too much of my time.
Let me know how you would like to proceed. I can add the changes in this PR, or submit a new one.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] astange1 commented on pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
astange1 commented on PR #512:
URL: https://github.com/apache/wicket/pull/512#issuecomment-1102823621
I just mistakenly pushed a 2nd commit into this pull request. I was going to make this a 2nd pull request, but it can go here too.
This 2nd commit greatly reduces the overhead of hashCode() computation in the rendering cycle as well. These hashcode computations, which make calls to Objects.hash() create a temporary Object[] array, autobox the boolean and integer values, and are expensive due to this overhead. I ran some tests on JDK17 and JDK19 using JMH and these Object[] creations are not elided in after inlining and after Escape Analysis. One of our simplest Ajax callbacks results in 140 hashcode calls to the Abstract class. In total, these changes avoid the creation of about 11KB of garbage memory in the server _per_mouse_click. This code also runs in ~1ns, compared to ~15ns for the original code (on a M1 Ultra Macbook, JDK19). On older JDKs, like JDK11, the original code is much more expensive by comparison.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] astange1 commented on pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
astange1 commented on PR #512:
URL: https://github.com/apache/wicket/pull/512#issuecomment-1102826557
@theigl I just saw your note. I've been using git for some time, but new to guthub. I'll have to take some time to figure out how to remove this 2nd commit and move it into a new PR. That was my original intent, and didn't expect the 2nd commit to show up in this PR.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl closed pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
theigl closed pull request #512: List.of() creates an immutable List instance, without a backing Array...
URL: https://github.com/apache/wicket/pull/512
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
theigl commented on PR #512:
URL: https://github.com/apache/wicket/pull/512#issuecomment-1103131265
> I figured I would do that one to start. If that simple PR was accepted then I would gladly do more, and if not then I hadn't wasted too much of my time.
That sounds good to me. Let's keep it simple. Also I just realized that `List.of` is not null-safe. So we probably can't use it as a universal replacement for header items anyway.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a diff in pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #512:
URL: https://github.com/apache/wicket/pull/512#discussion_r852819547
##########
wicket-core/src/main/java/org/apache/wicket/Component.java:
##########
@@ -2669,7 +2669,7 @@ public void internalRenderHead(final HtmlHeaderContainer container)
if (response.wasRendered(behavior) == false)
Review Comment:
The change itself looks good to me, but I'm wondering if the code is currently working as expected. We mark a pair of Component+Behavior as rendered, but the check if a behavior was already rendered only tests for the behavior itself.
@martin-g: You originally introduced this to fix https://issues.apache.org/jira/browse/WICKET-4570 many years ago. Could it be that you forgot to adjust the `wasRendered` condition? Imho the current code will never return `true` for `response.wasRendered(behavior)`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
theigl commented on PR #512:
URL: https://github.com/apache/wicket/pull/512#issuecomment-1102819891
@astange1: I like the hashcode improvements, because they showed up in my profiler as well. Please create a separate PR for this change though, so it is easier to review and discuss.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a diff in pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #512:
URL: https://github.com/apache/wicket/pull/512#discussion_r853396647
##########
wicket-core/src/main/java/org/apache/wicket/Component.java:
##########
@@ -2669,7 +2669,7 @@ public void internalRenderHead(final HtmlHeaderContainer container)
if (response.wasRendered(behavior) == false)
Review Comment:
+1 to fix it
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a diff in pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #512:
URL: https://github.com/apache/wicket/pull/512#discussion_r853131648
##########
wicket-core/src/main/java/org/apache/wicket/Component.java:
##########
@@ -2669,7 +2669,7 @@ public void internalRenderHead(final HtmlHeaderContainer container)
if (response.wasRendered(behavior) == false)
Review Comment:
It looks like a bug indeed!
But it is strange that it was uncovered for so many years.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [wicket] theigl commented on a diff in pull request #512: List.of() creates an immutable List instance, without a backing Array...
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #512:
URL: https://github.com/apache/wicket/pull/512#discussion_r853392588
##########
wicket-core/src/main/java/org/apache/wicket/Component.java:
##########
@@ -2669,7 +2669,7 @@ public void internalRenderHead(final HtmlHeaderContainer container)
if (response.wasRendered(behavior) == false)
Review Comment:
Should we fix this? Or should we get rid of the whole conditional and always call `behavior.renderHead` as we seem to have been doing for 10 years without issues. All tests pass in both solutions.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org