-
-
Notifications
You must be signed in to change notification settings - Fork 33
Fix 306 memory #345
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
base: master
Are you sure you want to change the base?
Fix 306 memory #345
Conversation
make CombinedJarIndex's EntryIndex a combination of MainJarIndex's and LibrariesJarIndex's
make CombinedJarIndex's ReferenceIndex a combination of MainJarIndex's and LibrariesJarIndex's; does NOT currently index references from main to lib
…al with public constructors again
…ex's and LibrariesJarIndex's
bb19a36 to
90978ff
Compare
ix0rai
left a comment
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.
looks good! we should look into making a test using a heavier jar to catch regressions like this in the future
enigma/src/main/java/org/quiltmc/enigma/api/analysis/index/jar/CombinedEntryIndex.java
Show resolved
Hide resolved
|
|
||
| String getTranslationKey(); | ||
|
|
||
| // TODO we should probably replace this with a type object in a breaking update |
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.
what are you thinking of for the new version?
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.
at minimum a Type<I extends JarIndex> object that's used as the key for the indexer
it would also hold the translation key
we could also use it to hide the actual indexing methods from users calling getIndex(Type<...> type):
change JarIndex like:
public interface JarIndex<A> {
//...
A getAccess();
record Type<A> {
//...
}
}then getIndex (possibly renamed to accessIndex or smth) would return the access, not the index itself
access types would provide the access methods like EntryIndex::getParameters but hide the indexing methods like EntryIndex::indexMethod
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.
nice, i love that idea of keeping things better isolated
|
forgot to de-draft earlier, de-drafted now |
Supersedes #344
Fixes excessive memory usage introduced in #306 by:
CombinedJarIndex'sEntyIndex,ReferenceIndex, andBridgeMethodIndexactual combinations ofMainJarIndex's andLibraryJarIndex's indexesEnigma'smainReferencedPredicate; it was just wasted overhead since the number of libs classes is tinyBREAKING not running custom indexers overrestored: performance is fine with it; avoids breaking indexers that need the combined scope all at onceCombinedJarIndex's classes; they already ran over main's and lib'sImplementing proper combining for just
EntyIndexandReferenceIndexwas enough to fix performance (I didEntryIndexfirst, so justReferenceIndexmight be enough); it ran fine before I combinedBridgeMethodIndexes.It's probably possible to combine
InheritanceIndexes, too, but it could be considerably more complex and might involve the combined index actually doing some of its own indexing.Same goes for
LambdaIndexes.BREAKING (maybe*): the combined
ReferenceIndexdoes not currently index references from the main jar to libs; it's just a combination of the main and libReferenceIndexes.*Not sure if this is actually true and not sure how to check.
minor breakage:
EntyIndex,ReferenceIndex, andBridgeMethodIndexinto sealed interfaces with hidden implementations; they're no longer instantiable or extendable by users (probably not intended to be instantiable or extendable anyways)CombinedJarIndexfinaland its constructorprivate(not intended for extension anyway)