-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
ArC: speed up bean resolution at runtime #33760
Conversation
This PR is related to #33693. |
Note that this is a runtime optimization. I didn't find a reasonable build-time alternative. Reasonable in terms of performance gain and complexity. CC @franz1981 @Sanne |
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.
LGTM. Memory overhead should be pretty low, because all the objects (except of the maps and lists) must exist anyway [1], but some measurements would be good.
[1] I'm actually not sure about the String
keys in the map. I'd personally use the Class
object as the key, because that must exist. Looking at JDK 11's java.lang.Class
, the getName()
method initializes the class name lazily, so that String
probably doesn't need to exist, but it's entirely possible we force it into existence elsewhere.
independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/ArcContainerImpl.java
Show resolved
Hide resolved
bd1f32b
to
22320ef
Compare
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 like this solution, it's IMO a good middle ground between what we had and going bonkers on over optimizing this code :)
@Ladicek Let's keep it this way for now. We could change it later, once we measure the RSS and find out that there's too much overhead... WDYT? |
I already approved, so I'm fine :-) |
22320ef
to
1ecd5bf
Compare
independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/ArcContainerImpl.java
Show resolved
Hide resolved
Very nice, thanks for looking at this so quickly! |
This comment has been minimized.
This comment has been minimized.
Specifically, the invocation of BeanManaget#getBeans() and the first hit for any programmatic lookup. We use a precomputed map of a raw type to the set of matching beans which reduces the number of beans we need to iterate over when we're looking for matching beans. Note that BeanManaget#getBeans() does not use the computing cache of results so this optimizaton results in a significant speed-up. On the other hand, the precomputed map could be memory demanding for large applications.
1ecd5bf
to
d582a45
Compare
This comment has been minimized.
This comment has been minimized.
Failing Jobs - Building d582a45
Full information is available in the Build summary check run. Failures⚙️ Native Tests - Amazon #- Failing: integration-tests/amazon-lambda integration-tests/amazon-lambda-http
📦 integration-tests/amazon-lambda✖
📦 integration-tests/amazon-lambda-http✖
|
The failures in the |
Specifically, the invocation of BeanManaget#getBeans() and the first hit for any programmatic lookup.
We use a precomputed map of a raw type to the set of matching beans which reduces the number of beans we need to iterate over when we're looking for matching beans.
Note that BeanManaget#getBeans() does not use the computing cache of results so this optimizaton results in a significant speed-up. On the other hand, the precomputed map could be memory demanding for large applications.