-
Notifications
You must be signed in to change notification settings - Fork 94
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
add IndexView.getKnownDirectSubinterfaces and getAllKnownSubinterfaces #184
Conversation
be28929
to
62cd8e8
Compare
This is technically a breaking change, but only for users who extend/implement the `IndexView` interface. Callers are fine. It is tempting to fix `IndexView.getKnownDirectImplementors` to no longer return subinterfaces, but we refrain from doing so to maintain behavioral compatibility. This change requires persistent format version bump. That has already happened on the `smallrye` branch, so this commit doesn't bump again, but note that this commit can't be backported to Jandex 2.x.
62cd8e8
to
6c0226a
Compare
* @return a non-null list of all known subinterfaces of interfaceName | ||
* @since 3.0 | ||
*/ | ||
default Collection<ClassInfo> getKnownDirectSubinterfaces(String interfaceName) { |
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.
Are these versions of the method really needed? I mean it can be useful yes, but all other methods accept DotName
params so one could ask "why string and class only for these two methods"? Also clients may prefer these versions of methods which is suboptiomal because (1) componentized names are preferred and (2) it's always better to share DotName
instances and avoid unnecessary allocations...
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.
All other methods also have a String
and Class
overloads since #155
When it comes to componentized DotName
s, I think of it as a two-way street. When storing DotName
s in a humongous data structure that must be as compact as possible (such as a Jandex index itself), using a componentized form is strongly preferred.
When it comes to one-off usages (such as these methods that lookup data from the index), it doesn't really matter in my opinion. If you share DotName
instances (typically by storing them into static final
fields), you prevent unnecessary allocations at the expense of higher old gen usage. If you create them on demand, you cause higher allocation rate. In this particular case, the freshly created DotName
instances are never stored anywhere, so they die very young, and collecting a new gen is rather cheap.
So I don't really think this is an issue.
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.
All other methods also have a
String
andClass
overloads since #155
Ok, didn't know about that one.
When it comes to componentized
DotName
s, I think of it as a two-way street. When storingDotName
s in a humongous data structure that must be as compact as possible (such as a Jandex index itself), using a componentized form is strongly preferred.
AFAIK the componentized version also has a bit more efficient equals()
...
When it comes to one-off usages (such as these methods that lookup data from the index), it doesn't really matter in my opinion. If you share
DotName
instances (typically by storing them intostatic final
fields), you prevent unnecessary allocations at the expense of higher old gen usage. If you create them on demand, you cause higher allocation rate. In this particular case, the freshly createdDotName
instances are never stored anywhere, so they die very young, and collecting a new gen is rather cheap.
Ok, I won't argue ;-) although I think that these methods make the API less readable and the "collecting a new gen is rather cheap" argumet is a bit disputable.
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.
Actually I had to fix DotName.equals()
for the case when both DotName
s are componentized, because the previous implementation assumed that componentization is canonical, which it is not (see #175 and #176 if you wanna know more, but beware, it's a long brain dump of mine :-) ). So DotName.equals()
is actually most efficient when both DotName
s are not componentized, because that's just a string comparison.
Performance disputes are meaningless without measurements, and I don't have any -- my only argument is that Stuart clearly thought it's fine in #155 :-)
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.
Actually I had to fix
DotName.equals()
for the case when bothDotName
s are componentized, because the previous implementation assumed that componentization is canonical, which it is not (see #175 and #176 if you wanna know more, but beware, it's a long brain dump of mine :-) ). SoDotName.equals()
is actually most efficient when bothDotName
s are not componentized, because that's just a string comparison.
Interesting!
Performance disputes are meaningless without measurements, and I don't have any -- my only argument is that Stuart clearly thought it's fine in #155 :-)
+1 :D
This is technically a breaking change, but only for users who
extend/implement the
IndexView
interface. Callers are fine.It is tempting to fix
IndexView.getKnownDirectImplementors
to no longer return subinterfaces, but we refrain from doing so
to maintain behavioral compatibility.
This change requires persistent format version bump. That has already
happened on the
smallrye
branch, so this commit doesn't bumpagain, but note that this commit can't be backported to Jandex 2.x.
Resolves #65