-
Notifications
You must be signed in to change notification settings - Fork 484
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
Lambdas for proxies take2 #686
base: 2.x
Are you sure you want to change the base?
Lambdas for proxies take2 #686
Conversation
…xies This is a re-factor of the code reverted in PR Netflix#685, originally by @kilink
I've also realized that, since a ConfigProxyFactory is always initialized after the config object it proxies ... we could have a flag to disable use of lambdas! So anyone hit by the weirdness can just set the flag and keep on leaking proxies to their hearts contents 😁 |
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.
I remember loving this project, and randomly took a look since such an interesting situation. Pay no mind to my unsolicited 2p unless you care to!
} else { // Java 17 onwards | ||
INSTANCE = new LambdasFactory(); | ||
} | ||
LOG.info("Choosing {} as invoker factory for default methods", INSTANCE.getClass()); |
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.
is info or debug more sensible for this?
|
||
// The threshold is simply a best guess. Creating 1000 lambdas for the same interface is almost certainly | ||
// a leak, right? | ||
private static final int MAX_SEEN_THRESHOLD = 1000; |
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.
would a JMH bench trigger stuff like this?
* provides a noticeable performance boost. This implementation is known to fail in java 9 - 11. It *should* work | ||
* from 12 or 13 onwards, but we have only tested it in 17 and 21. | ||
*/ | ||
private static class LambdasFactory extends DefaultMethodInvokerFactory { |
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.
if package-scoped (final, too) you could make a matrix test and benchmarks, or even fuzzing to rely less on seen in usage stuff.
This PR re-introduces the usage of lambdas for config proxies. I refactored things into a separate class and split the implementations for different JVMs to make things cleaner. I also introduced a "usage counter" circuit breaker to reduce the chance that users could hit OOM errors like they did before. I'll probably end up removing that later, thought.
There's also a bunch of compiler warning cleanups. I tried to keep those on a separate commit so the actual change is easier to review.