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

[1.20.x] Trivial BackgroundScanHandler optimisations #9713

Conversation

PaintNinja
Copy link
Contributor

  • Comment out three private lists that are updated but never queried, with no getters. Presumably some left over debug code?
  • Use .toList() instead of .collect(Collectors.toList()) and !str.isEmpty() instead of str.length() > 0 where appropriate

@autoforge autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.20 labels Sep 3, 2023
@autoforge autoforge bot requested a review from a team September 3, 2023 19:56
@Technici4n
Copy link
Contributor

Are there any numbers for these "optimizations"?

@PaintNinja PaintNinja changed the title [1.20.1] Trivial BackgroundScanHandler optimisations [1.20.x] Trivial BackgroundScanHandler optimisations Sep 21, 2023
@LexManos
Copy link
Member

This is more a code cleanup then an Optimization. You save a bit of memory removing the lists.
isEmpty vs length() saves you one rshift operation. So not important
But the toList one actually could be important depending on the stream implementation, could be a lot faster.

HOWEVER, the PROPER solution if you care about performance is to get rid of cpw's over-engineered code that does nothing but waste cycles, as the Filter matches everything , And the ModAnnoation intermediate object could be streamlines a lot. And you can bypass the entire stream if you use a basic for loop to bake the object. Or you used a builder system and baked it in the Annotation visitor's end.

This is one of the many places that I plan on cleaning up in the future when i have the free time to do the full project audit.

@LexManos LexManos closed this in 05c88ae Sep 28, 2023
@PaintNinja
Copy link
Contributor Author

PaintNinja commented Sep 28, 2023

Finally figured out profiling and got some numbers. Aside from Lex's comments on these changes, the removal of redundant list operations does make a measurable difference:
image

Most notably, addCompletedFile() is about 60% faster. Of course, doing larger changes like those suggested would yield in bigger improvements across the board, but considering how trivial these optimisations are, it's not bad.

RealMangorage pushed a commit to RealMangorage/MinecraftForge that referenced this pull request Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Triage This request requires the active attention of the Triage Team. Requires labelling or reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants