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

optimize by localising math functions #83

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tenplus1
Copy link
Member

optimizations by localising math functions.

localise math functions
localise math functions
localise math functions
localise math function
@SmallJoker
Copy link
Member

Do these changes make a measurable performance difference on your side? How big of a difference is it?

@tenplus1
Copy link
Member Author

@SmallJoker - A few ms here and there, although Nether generation does feel a lot smoother.

@Treer
Copy link
Collaborator

Treer commented Dec 21, 2024

I made the original local math function code that tenplus1 is adding to.

It was long enough ago that I don't recall how much speed testing I did or which lua implementation it was on. Any testing I'd done would have been in an earlier (not-nether) mod that used the same kind of mapgen. iirc I was only bothering to make local math functions if an operation was called many times in an inner loop.

The big reason to avoid potentially-premature optimization is the added code complexity/legibility/maintenance, and I don't think this applies to tenplus1's changes because they are extremely readable, and also consistent with how the code is already written, enough that they probably make the code more consistent and predictable.

So I can't say I have a problem with these changes, regardless of whether there's a noticeable performance difference. (as nice as it would be to know whether nether go faster)

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.

3 participants