New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Save promises type pollution due to interface type checks #12980
Conversation
FYI, the relevant guilty part is the cast to
Adding a new field won't impact instance footprint before
After:
Thanks to pad, the added field won't cost us an increased mem footprint. |
53b46fb
to
fc24a74
Compare
Motivation: DefaultPromise performs interfaces type checks to distinguish single/multi listeners presence, hitting https://bugs.openjdk.org/browse/JDK-8180450 Modifications: Using separate listener fields that won't require type checks to evaluate listeners arity Result: No type pollution for user-defined listeners types.
fc24a74
to
91d137f
Compare
listener = null; | ||
} else if (listeners != null) { | ||
listeners.remove(toRemove); | ||
// TODO we need ot compact it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normanmaurer
The compaction can happen while moving from 2 to 1 element too, but given that the listeners collection is already allocated won't worths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's not bother with compaction. Removing listeners is so rare, and as you say, we already allocated the collection. Not to mention, if listeners are repeatedly added and removed, we don't want to thrash between the two modes and produce garbage on every switch.
} else if (listeners instanceof GenericProgressiveFutureListener) { | ||
return listeners; | ||
} else if (listener instanceof GenericProgressiveFutureListener) { | ||
// TODO can we save the instanceof and just makes use of the progressiveSize? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Progressive promises and futures are not used very often, so fine to not optimize IMO.
The PR is ready to be reviewed, is still on draft because I'm waiting my collagues to test is in a proper end 2 end test |
@@ -55,7 +55,8 @@ | |||
* | |||
* Threading - synchronized(this). We must support adding listeners when there is no EventExecutor. | |||
*/ | |||
private Object listeners; | |||
private GenericFutureListener<? extends Future<?>> listener; | |||
private DefaultFutureListeners listeners; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI we used Object
to be able to save space in the past. That said I think this is not a good reason for this anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eheh I believe so: I've reported on #12980 (comment) the actual size by using this super nice IDEA plugin: https://plugins.jetbrains.com/plugin/10953-jol-java-object-layout
@normanmaurer how I can trigger again new tests? I don't know if I got such super powers |
@franz1981 do you see the "re-run tests button on the right top corner" ? https://github.com/netty/netty/actions/runs/3426801410/jobs/5709114178 |
thanks @normanmaurer , yep! rerun now |
Now CI looks much better :) @normanmaurer I've placed some TODO to get feedbacks on other code paths, let me know if I can rid of them and open a separate pr instead, to move this forward |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had opinions on the TODOs, but otherwise looks good.
listener = null; | ||
} else if (listeners != null) { | ||
listeners.remove(toRemove); | ||
// TODO we need ot compact it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's not bother with compaction. Removing listeners is so rare, and as you say, we already allocated the collection. Not to mention, if listeners are repeatedly added and removed, we don't want to thrash between the two modes and produce garbage on every switch.
} else if (listeners instanceof GenericProgressiveFutureListener) { | ||
return listeners; | ||
} else if (listener instanceof GenericProgressiveFutureListener) { | ||
// TODO can we save the instanceof and just makes use of the progressiveSize? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Progressive promises and futures are not used very often, so fine to not optimize IMO.
Motivation:
DefaultPromise performs interfaces type checks to distinguish single/multi listeners presence, hitting https://bugs.openjdk.org/browse/JDK-8180450
Modifications:
Using separate listener fields that won't require type checks to evaluate listeners arity
Result:
No type pollution for user-defined listeners types.