Skip to content

Various tests, tweaks and optimisations#109

Open
MotionlessTrain wants to merge 6 commits into
Mojang:masterfrom
MotionlessTrain:tweaks/optimisations
Open

Various tests, tweaks and optimisations#109
MotionlessTrain wants to merge 6 commits into
Mojang:masterfrom
MotionlessTrain:tweaks/optimisations

Conversation

@MotionlessTrain

Copy link
Copy Markdown

Closes #108 (though I am still curious about the .map(Function.identity()))

Proposed changes

  • Adds a (non-exhaustive) test suit for datafixers and the optimisation rules, to help testing the rest of the changes in the PR
  • Fixes some typings in some rewrite rules. Theoretically, with a lot of optimisations after one another, some types could have been very off for some optics
    • In case of SortProj and SortInj, as the internals didn't seem to agree which order the functions were first in, I decided to keep the runtime behaviour (i.e. the order in doRewrite) intact, rewriting the other rule
    • I happened to notice LensComp could have some typing issues too, when constructing the large optimisation test, due to defaulting to the first optic. The result type is correct (so checks on whether the output of the datafix has the correct type are correct, but the input type was wrong
  • Cache a possibly quite expensive rule calculation, to only be executed once, per recursive index
    • As I wrote on the issue, as rules could theoretically target and rewrite a CheckType, I didn't there to optimise independent on index, even though that practically, that would not be an issue
  • Fix the hmac function in CheckType to actually check the index. That makes the returned RewriteRule a bit more efficient, as the rule branches where the check type failed wouldn't be used anyway
  • Optimise a few cases where on an Either or DataResult an optional was constructed, to just be checked for being present, to use a faster boolean or instanceof check

@hiSandog

hiSandog commented Jul 3, 2026

Copy link
Copy Markdown

The added optimisation tests are the most valuable part of this PR. Since several fixes preserve runtime behaviour while changing inferred optic/input types, it may be worth naming a few tests around the invariant being protected: output value equality, output type equality, and input type stability. That would make later reviewers less likely to remove a test because it appears to only exercise a no-op optimisation chain.

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.

Several minor (performance) bugs, or oddities

2 participants