Skip to content

Conversation

@SirYwell
Copy link
Contributor

Trying to re-use TreePreviewCraftingCalculatorListener to calculate the normal Preview too, as that seems to have better performance characteristics.

@SirYwell
Copy link
Contributor Author

@raoulvdberge what do you think of this in general? Currently the order is different, I didn't take a closer look if that can be fixed easily. But it seems to be faster and would render the PreviewCraftingCalculatorListener and related code unnecessary.

@raoulvdberge
Copy link
Contributor

No, I'm not a fan of this. The code should match the intent, and now you are reusing tree code for list code because it is faster. I'd like to know WHY it's faster, and use that knowledge to improve the list code.

I have no problem with having both list and tree code.

For me the ordering in the list is important.

I don't want the list view to become dependant on the tree view, the whole listener system is set in place to avoid that.
If lists end up getting different requirements in the future, we'll be in a world of hurt to retrofit the tree code to support it too.

@SirYwell
Copy link
Contributor Author

SirYwell commented Oct 29, 2025

Understandable.
The main bottleneck from the PreviewBuilder is the copy, because we copy a lot of data many times with just little changes.
This isn't a problem with MutableTreePreviewNode. That's why it is faster when reducing the MutableTreePreviewNode at the end to a Preview.

Maybe the concept can be used for PreviewBuilder too, e.g., remember the relevant PreviewBuilder instances and instead of copying their data each time, merge them at the end. But that also might have difficulties when we want to maintain the exact same order, not sure. Other alternatives would include using immutable collections, but they typically have asymptotically slower access and the value would need to be immutable too, so not sure if there's any benefit from that (and it would require an additional dependency).

@SirYwell SirYwell force-pushed the perf/NO-ISSUE/faster-preview branch from 4117ee3 to 56ea26b Compare October 29, 2025 19:46
@SirYwell
Copy link
Contributor Author

I now reworked the implementation of PreviewBuilder directly to avoid the copy and remember a trace of PreviewBuilders. This allows us to do all the necessary computation in one step at the end (but in theory at any moment, for that current state). The correct order should be maintained now too if I didn't miss something.

I didn't test this beyond existing unit tests for now. I also didn't profile it.

@SirYwell SirYwell changed the title perf: faster Preview calculation using TreePreviewCraftingCalculatorListener perf: lazily compute PreviewBuilder state Oct 29, 2025
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.

2 participants