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 16:27:01 UTC
[GitHub] [wicket] astange1 opened a new pull request, #513: More efficient hashcode computations
astange1 opened a new pull request, #513:
URL: https://github.com/apache/wicket/pull/513
AbstractJavaScriptReferenceHeaderItem and children use a simple, but somewhat expensive, hashcode computation. We can obtain the same result using much less time, and avoid all creation of garbage objects. This is a substantial memory savings on complex pages with a large component hierarchy.
--
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 #513: WICKET-6977 More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r860833001
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,11 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // Not using `Objects.hash` for performance reasons
+ int result = super.hashCode();
Review Comment:
@martin-g: I have consolidated the super calls to equals/hashCode in both abstract base classes.
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r857113266
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,7 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ return (charset != null ? charset.hashCode() << 2 : 0)
Review Comment:
Please use an auto-generated hash-code for this method as well. Doing everything consistently, makes it easier to simply regenerate the hash-code should a new field be added.
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/CssReferenceHeaderItem.java:
##########
@@ -154,7 +154,13 @@ private String getUrl()
@Override
public int hashCode()
{
- return Objects.hash(super.hashCode(), reference, getMedia(), pageParameters, getRel());
+ String s;
+ int result = super.hashCode();
+ result = 31*result + ((reference != null) ? reference.hashCode() : 0);
Review Comment:
There should be whitespace around the multiplier. Please apply our standard code format.
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/CssReferenceHeaderItem.java:
##########
@@ -154,7 +154,13 @@ private String getUrl()
@Override
public int hashCode()
{
- return Objects.hash(super.hashCode(), reference, getMedia(), pageParameters, getRel());
+ String s;
Review Comment:
I'm not a fan of re-using this `String s`. Did you IDE generate it this way?
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on PR #513:
URL: https://github.com/apache/wicket/pull/513#issuecomment-1107905893
Please create a ticket for this issue in our [Jira](https://issues.apache.org/jira/browse/WICKET) with your original observations, so we have the documentation and a reference for the release notes.
I think we should add a brief comment for all the new hashCode implementations, so that doesn't get changed back accidentally in the future. Something like:
```// Not using `Objects.hash` for performance reasons```
Otherwise, this looks good to me now.
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r853418294
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,16 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // original code was: return Objects.hash(async, defer, charset);
+ // This returns the same hash code value, but doesn't allocate an Object[3],
+ // which consumes 32 bytes, and doesn't need to autobox the two booleans.
+ // This method is called very often, especially when a containing Map or Set is resized,
+ // and this version will run faster and save a lot of memory.
+ // It is possible that Escape Analysis would elide the creation of the Object[],
+ // but we do not see that in our running applications with the latest JDK.
+ return 31*31*31 +
+ 31*31*Boolean.hashCode(async) +
+ 31*Boolean.hashCode(defer) +
+ ((charset != null) ? charset.hashCode() : 0);
Review Comment:
I think it is OK to return different hashcode values!
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
astange1 commented on PR #513:
URL: https://github.com/apache/wicket/pull/513#issuecomment-1107890271
Hello, I believe I have made all the code formatting chnages that were requested. I also made an additional update to the hashcode computation for a ResourceReference class, which should result in improved JMH performance.
@theigl The remaining memory allocation in the posted JMH results should be resolved now. Can you kindly re-run the results with this updated code to confirm? My JMH setup is running on a custom JDK19 build, and it would be a pain for me to set up here for Wicket.
FYI. Wicket-utils has an Objects.hashCode() implementation that should be removed in favor of using the one in the JDK. It is used by the Model class.
--
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 #513: WICKET-6977 More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r860819542
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,11 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // Not using `Objects.hash` for performance reasons
+ int result = super.hashCode();
Review Comment:
I don't remember now but I guess those were generated by my IDE.
I agree that super should be called!
--
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 #513: WICKET-6977 More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r860679457
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,11 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // Not using `Objects.hash` for performance reasons
+ int result = super.hashCode();
Review Comment:
I just noticed some inconsistencies:
You added `super.hashCode` here, but not to `AbstractCssReferenceHeaderItem`. We should probably keep those two classes in sync.
And the bigger issue: The contracts for equals and hashCode now don't align. If we add `super.hashCode` (which imho we should, because the super class contains `Objects.hash(markupId)`), we should also add `markupId` to the equals contract.
@martin-g: Do you have any idea, why `AbstractJavaScriptReferenceHeaderItem` and `AbstractCssReferenceHeaderItem` do not currently user `super.equals` and `super.hashCode`. Do we intentionally ignore the `markupId` field in the super class? Your original commit for these implementations was 47aa1461d58b9a5660fc11032db952ce4df36a33.
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
astange1 commented on PR #513:
URL: https://github.com/apache/wicket/pull/513#issuecomment-1108025878
See https://issues.apache.org/jira/browse/WICKET-6977
I will add comments to the updated hashCode methods tomorrow.
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
astange1 commented on PR #513:
URL: https://github.com/apache/wicket/pull/513#issuecomment-1107898196
I just pushed changed to address the formatting and the two additional files.
Hotspot is able to ellide the OIbject[] creation for calls to Objects.hash() with a single object, so those instances will not benefit from rewriting. It might be better to use Objects.hashCode() though, as the Object[] ellision probably doesn't happen until the C2 compiler is able to fully optimize the code.
--
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 #513: WICKET-6977 More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r860733685
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,11 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // Not using `Objects.hash` for performance reasons
+ int result = super.hashCode();
Review Comment:
If we are unsure about this, I'll simply remove the `super.hashCode` from the new implementation to maintain the current 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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on PR #513:
URL: https://github.com/apache/wicket/pull/513#issuecomment-1107827128
To give some more context, I ran the benchmarks myself. The overhead of `Objects.hash` is very significant:
```
Benchmark Mode Cnt Score Error Units
Benchmarks.hashCodeNew thrpt 9 134406237,465 ± 712957,234 ops/s
Benchmarks.hashCodeNew:·gc.alloc.rate.norm thrpt 9 40,004 ± 0,001 B/op
Benchmarks.hashCodeNew:·gc.churn.G1_Eden_Space.norm thrpt 9 40,115 ± 0,401 B/op
Benchmarks.hashCodeNew:·gc.churn.G1_Survivor_Space.norm thrpt 9 ≈ 10⁻⁴ B/op
Benchmarks.hashCodeOld thrpt 9 29199877,793 ± 1543709,148 ops/s
Benchmarks.hashCodeOld:·gc.alloc.rate.norm thrpt 9 120,015 ± 0,004 B/op
Benchmarks.hashCodeOld:·gc.churn.G1_Eden_Space.norm thrpt 9 120,181 ± 1,556 B/op
Benchmarks.hashCodeOld:·gc.churn.G1_Survivor_Space.norm thrpt 9 0,001 ± 0,001 B/op
```
The new hashcodes are **4x** faster and allocation is reduced significantly.
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r853466355
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,16 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // original code was: return Objects.hash(async, defer, charset);
+ // This returns the same hash code value, but doesn't allocate an Object[3],
+ // which consumes 32 bytes, and doesn't need to autobox the two booleans.
+ // This method is called very often, especially when a containing Map or Set is resized,
+ // and this version will run faster and save a lot of memory.
+ // It is possible that Escape Analysis would elide the creation of the Object[],
+ // but we do not see that in our running applications with the latest JDK.
+ return 31*31*31 +
+ 31*31*Boolean.hashCode(async) +
+ 31*Boolean.hashCode(defer) +
+ ((charset != null) ? charset.hashCode() : 0);
Review Comment:
> Are we OK with the hashcode values being different, perhaps resulting in other changes due to hashing?
Good question. My guess would be that it doesn't matter, since collecting header items and putting them in collections is always done on a single JVM in a single request. Does anyone else see a problem with changing the hash codes?
--
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] solomax commented on a diff in pull request #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
solomax commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r853968190
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,16 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // original code was: return Objects.hash(async, defer, charset);
+ // This returns the same hash code value, but doesn't allocate an Object[3],
+ // which consumes 32 bytes, and doesn't need to autobox the two booleans.
+ // This method is called very often, especially when a containing Map or Set is resized,
+ // and this version will run faster and save a lot of memory.
+ // It is possible that Escape Analysis would elide the creation of the Object[],
+ // but we do not see that in our running applications with the latest JDK.
+ return 31*31*31 +
+ 31*31*Boolean.hashCode(async) +
+ 31*Boolean.hashCode(defer) +
+ ((charset != null) ? charset.hashCode() : 0);
Review Comment:
`hash` method from here https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html can also be used :)
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r853384634
##########
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)
{
behavior.renderHead(this, response);
- List<IClusterable> pair = Arrays.asList(this, behavior);
Review Comment:
Please revert the commit from this PR.
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,16 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // original code was: return Objects.hash(async, defer, charset);
+ // This returns the same hash code value, but doesn't allocate an Object[3],
+ // which consumes 32 bytes, and doesn't need to autobox the two booleans.
+ // This method is called very often, especially when a containing Map or Set is resized,
+ // and this version will run faster and save a lot of memory.
+ // It is possible that Escape Analysis would elide the creation of the Object[],
+ // but we do not see that in our running applications with the latest JDK.
+ return 31*31*31 +
+ 31*31*Boolean.hashCode(async) +
+ 31*Boolean.hashCode(defer) +
+ ((charset != null) ? charset.hashCode() : 0);
Review Comment:
I find this quite hard to read. Can you please use IntelliJ or Eclipse to generate the code. IntelliJ looks like this:
```
@Override
public int hashCode()
{
int result = super.hashCode();
result = 31 * result + (async ? 1 : 0);
result = 31 * result + (defer ? 1 : 0);
result = 31 * result + (charset != null ? charset.hashCode() : 0);
return result;
}
```
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,16 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // original code was: return Objects.hash(async, defer, charset);
+ // This returns the same hash code value, but doesn't allocate an Object[3],
+ // which consumes 32 bytes, and doesn't need to autobox the two booleans.
+ // This method is called very often, especially when a containing Map or Set is resized,
+ // and this version will run faster and save a lot of memory.
+ // It is possible that Escape Analysis would elide the creation of the Object[],
+ // but we do not see that in our running applications with the latest JDK.
Review Comment:
This comment should go into the corresponding Jira ticket. A simple comment like this should be enough:
```
// Not using `Objects.hash` for performance reasons
```
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,16 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // original code was: return Objects.hash(async, defer, charset);
+ // This returns the same hash code value, but doesn't allocate an Object[3],
+ // which consumes 32 bytes, and doesn't need to autobox the two booleans.
+ // This method is called very often, especially when a containing Map or Set is resized,
+ // and this version will run faster and save a lot of memory.
+ // It is possible that Escape Analysis would elide the creation of the Object[],
+ // but we do not see that in our running applications with the latest JDK.
+ return 31*31*31 +
+ 31*31*Boolean.hashCode(async) +
+ 31*Boolean.hashCode(defer) +
+ ((charset != null) ? charset.hashCode() : 0);
Review Comment:
I find this quite hard to read. Can you please use IntelliJ or Eclipse to generate the code. IntelliJ looks like this:
```java
@Override
public int hashCode()
{
int result = super.hashCode();
result = 31 * result + (async ? 1 : 0);
result = 31 * result + (defer ? 1 : 0);
result = 31 * result + (charset != null ? charset.hashCode() : 0);
return result;
}
```
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,16 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // original code was: return Objects.hash(async, defer, charset);
+ // This returns the same hash code value, but doesn't allocate an Object[3],
+ // which consumes 32 bytes, and doesn't need to autobox the two booleans.
+ // This method is called very often, especially when a containing Map or Set is resized,
+ // and this version will run faster and save a lot of memory.
+ // It is possible that Escape Analysis would elide the creation of the Object[],
+ // but we do not see that in our running applications with the latest JDK.
Review Comment:
This comment should go into the corresponding Jira ticket. A simple comment like this should be enough:
```
// Not using `Objects.hash` for performance reasons
```
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r857161161
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/JavaScriptContentHeaderItem.java:
##########
@@ -94,6 +94,8 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(super.hashCode(), javaScript);
+ int result = super.hashCode();
+ result = 31*result + ((javaScript != null) ? javaScript.hashCode() : 0);
Review Comment:
Please adjust the formatting.
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/CssUrlReferenceHeaderItem.java:
##########
@@ -97,7 +97,12 @@ public String toString()
@Override
public int hashCode()
{
- return Objects.hash(super.hashCode(), url, getMedia(), getRel());
+ String s;
+ int result = super.hashCode();
+ result = 31*result + ((url != null) ? url.hashCode() : 0);
+ result = 31*result + (((s = getMedia()) != null) ? s.hashCode() : 0);
+ result = 31*result + (((s = getRel()) != null) ? s.hashCode() : 0);
+ return result;
Review Comment:
Please remove the `s` variable and fix the formatting.
--
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 #513: WICKET-6977 More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on PR #513:
URL: https://github.com/apache/wicket/pull/513#issuecomment-1113238968
@astange1: This has been merged and backported to 9.x Thanks again!
If you identify more allocation hotspots in the future, please feel free to create more PRs.
--
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 a diff in pull request #513: WICKET-6977 More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
astange1 commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r860822148
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,11 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // Not using `Objects.hash` for performance reasons
+ int result = super.hashCode();
Review Comment:
@theigl Thank you for taking the final touches over. I think that is for the best as I am not that familiar with this code base.
I don't see any other needless allocation hotspots now.
Thank you!
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on PR #513:
URL: https://github.com/apache/wicket/pull/513#issuecomment-1107827819
@astange1: Given my benchmark results, we should definitely go ahead with these improvements.
I think we should apply them for *all* header items and in the most consistent way possible. I would vote for auto-generating all of the hashcodes without attempting any clever optimizations.
--
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 merged pull request #513: WICKET-6977 More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl merged PR #513:
URL: https://github.com/apache/wicket/pull/513
--
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 a diff in pull request #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
astange1 commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r853408043
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,16 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // original code was: return Objects.hash(async, defer, charset);
+ // This returns the same hash code value, but doesn't allocate an Object[3],
+ // which consumes 32 bytes, and doesn't need to autobox the two booleans.
+ // This method is called very often, especially when a containing Map or Set is resized,
+ // and this version will run faster and save a lot of memory.
+ // It is possible that Escape Analysis would elide the creation of the Object[],
+ // but we do not see that in our running applications with the latest JDK.
+ return 31*31*31 +
+ 31*31*Boolean.hashCode(async) +
+ 31*Boolean.hashCode(defer) +
+ ((charset != null) ? charset.hashCode() : 0);
Review Comment:
I wrote it the original way to produce the exact same hashcode value as the original code, and to minimize the math being done. Are we OK with the hashcode values being different, perhaps resulting in other changes due to hashing?
I will update the CSSHeaderItem classes, and also remove the comments.
--
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 #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on PR #513:
URL: https://github.com/apache/wicket/pull/513#issuecomment-1107848823
> @theigl I think you've already been asked about it, but I'd like to know how do you create these micro-benchmarks. Do you use any particular framework?
These are very simple JMH benchmarks. I already promised to create a `wicket-benchmarks` module where we can collect these benchmarks, but I haven't gotten around to it yet.
This specific benchmark looks like this:
```java
import org.apache.wicket.markup.head.JavaScriptReferenceHeaderItem;
import org.apache.wicket.markup.head.JavaScriptReferenceHeaderItemNew;
import org.apache.wicket.request.mapper.parameter.PageParameters;
import org.apache.wicket.resource.aggregator.ResourceReferenceA;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.infra.Blackhole;
import org.openjdk.jmh.runner.Runner;
import org.openjdk.jmh.runner.RunnerException;
import org.openjdk.jmh.runner.options.Options;
import org.openjdk.jmh.runner.options.OptionsBuilder;
import org.openjdk.jmh.runner.options.TimeValue;
@State(value = Scope.Thread)
public class HeaderItemBenchmarks {
private final JavaScriptReferenceHeaderItem item = new JavaScriptReferenceHeaderItem(new ResourceReferenceA(), new PageParameters(), "anyId");
private final JavaScriptReferenceHeaderItemNew item2 = new JavaScriptReferenceHeaderItemNew(new ResourceReferenceA(), new PageParameters(), "anyId");
@Benchmark
public void hashCodeOld(Blackhole blackhole)
{
blackhole.consume(item.hashCode());
}
@Benchmark
public void hashCodeNew(Blackhole blackhole)
{
blackhole.consume(item2.hashCode());
}
public static void main(String[] args) throws RunnerException
{
final Options opt = new OptionsBuilder()
.include(".*" + HeaderItemBenchmarks.class.getSimpleName() + ".*.hash.*")
.addProfiler("gc")
.warmupIterations(3)
.warmupTime(TimeValue.seconds(3))
.measurementIterations(3)
.measurementTime(TimeValue.seconds(3))
.threads(1)
.forks(3)
.build();
new Runner(opt).run();
}
}
```
--
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] bitstorm commented on pull request #513: More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
bitstorm commented on PR #513:
URL: https://github.com/apache/wicket/pull/513#issuecomment-1107845154
> @astange1: Given my benchmark results, we should definitely go ahead with these improvements.
>
> I think we should apply them for _all_ header items and in the most consistent way possible. I would vote for auto-generating all of the hashcodes without attempting any clever optimizations.
@theigl I think you've already been asked about it, but I'd like to know how do you create these micro-benchmarks. Do you use any particular framework?
--
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 #513: WICKET-6977 More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
astange1 commented on PR #513:
URL: https://github.com/apache/wicket/pull/513#issuecomment-1111619239
I'm sorry for the delay. I got a little busy with some other obligations. I believe I have added comments to all the impacted hashCode methods.
I believe this work is now complete. Please let me know if I missed something or if there is some other addition needed for this change.
--
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 #513: WICKET-6977 More efficient hashcode computations
Posted by GitBox <gi...@apache.org>.
theigl commented on code in PR #513:
URL: https://github.com/apache/wicket/pull/513#discussion_r860679457
##########
wicket-core/src/main/java/org/apache/wicket/markup/head/AbstractJavaScriptReferenceHeaderItem.java:
##########
@@ -135,6 +135,11 @@ public boolean equals(Object o)
@Override
public int hashCode()
{
- return Objects.hash(async, defer, charset);
+ // Not using `Objects.hash` for performance reasons
+ int result = super.hashCode();
Review Comment:
I just noticed some inconsistencies:
You added `super.hashCode` here, but not to `AbstractCssReferenceHeaderItem`. We should probably keep those two classes in sync.
And the bigger issue: The contracts for equals and hashCode now don't align. If we add `super.hashCode` (which imho we should, because the super class contains `Objects.hash(markupId)`), we should also add `markupId` to the equals contract.
@martin-g: Do you have any idea, why `AbstractJavaScriptReferenceHeaderItem` and `AbstractCssReferenceHeaderItem` do not currently use `super.equals` and `super.hashCode`. Do we intentionally ignore the `markupId` field in the super class? Your original commit for these implementations was 47aa1461d58b9a5660fc11032db952ce4df36a33.
--
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