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 startup on versionLessThan function #2979

Merged
merged 5 commits into from
Jul 11, 2024
Merged

Conversation

felipepasc
Copy link
Contributor

Hi, how are you? I've been using the lib for a few months now, but I notice a slowdown when trying to start, and when analyzing and testing I realized that most of it is because of the versionLessThan function, so I redid it in bash script, did some tests and everything worked fine.

@felipepasc
Copy link
Contributor Author

Wtf error?

@felipepasc felipepasc changed the title Optimize startup in versionLessThan function Optimize startup on versionLessThan function Jul 10, 2024
Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, this used to be implemented in bash. The new implementation was fine until the log4j mess and then like you found it slowed down with all those version checks.

I have a review comment. What did you mean by your last comment?

@felipepasc
Copy link
Contributor Author

I had gotten a weird Docker error, so I just added a space to run it again and it worked, I changed it to the version variable to check

@felipepasc felipepasc requested a review from itzg July 10, 2024 21:14
Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great now. Ready for me to merge?

BTW, what kind of startup time improvements did you notice from the change?

@felipepasc
Copy link
Contributor Author

From my tests on my dedicated device, before the change, the startup message took around 17 seconds to arrive, after the change it dropped to 7 seconds

@felipepasc
Copy link
Contributor Author

This number may not be the same for everyone, some will feel the changes more and others less depending on the CPU and especially the speed of the ra memory.

@felipepasc
Copy link
Contributor Author

But everything is ok now I believe

@itzg itzg merged commit 5baf398 into itzg:master Jul 11, 2024
5 checks passed
sevenrats pushed a commit to sevenrats/docker-minecraft-server that referenced this pull request Nov 19, 2024
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.

None yet

2 participants