Skip to content
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

Avoid calling superclass matcher where possible #1248

Merged
merged 1 commit into from
Feb 22, 2020

Conversation

mar-kolya
Copy link
Contributor

Saves about 300ms of startup time on test app

@mar-kolya mar-kolya requested a review from a team as a code owner February 21, 2020 19:02
Copy link
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a safe change.

.and(
safeExtendsClass(
named("com.netflix.hystrix.HystrixCommand")
.or(named("com.netflix.hystrix.HystrixObservableCommand"))));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of these are safe, but many aren't. For example, HystrixCommand is intended to be extended, so most usages of this won't match the package prefix. I'm guessing many of these others have a similar issue.

@@ -31,7 +32,9 @@

@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return not(isInterface()).and(safeHasInterface(named("org.hibernate.Criteria")));
return nameStartsWith("org.hibernate.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this one can be done. I've seen custom Criteria classes before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverged whole hibernate and a few others

@mar-kolya mar-kolya force-pushed the mar-kolya/reduce-superclass-matcher-exposure branch 4 times, most recently from ef575a7 to 91a56b3 Compare February 21, 2020 19:49
Saves about 100ms of startup time on test app
@mar-kolya mar-kolya force-pushed the mar-kolya/reduce-superclass-matcher-exposure branch from 91a56b3 to b6d374e Compare February 21, 2020 20:08
Copy link
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems more reasonable. Thanks for revisiting.

@mar-kolya mar-kolya merged commit 833f678 into master Feb 22, 2020
@mar-kolya mar-kolya deleted the mar-kolya/reduce-superclass-matcher-exposure branch February 22, 2020 02:16
@dougqh
Copy link
Contributor

dougqh commented Feb 25, 2020

The change seems like a nice improvement overall, but I'm wondering if we need to adjust our level of abstraction at some point -- not for this PR.

Specifically, I'm wondering if we should have a method on the Instrumenter that describes whether to search just a subset of packages.

I also think we could stand to create some helper methods for common idioms like...
not(isInterface()).and(hasSuper(...))

@randomanderson randomanderson added this to the 0.44.0 milestone Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants