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

Additional checks for unsigned integers to avoid underflow and overflow #3197

Merged
merged 2 commits into from
Feb 6, 2022

Conversation

nekiro
Copy link
Member

@nekiro nekiro commented Oct 23, 2020

Additional checks in getNumber(L, arg)

Prints a handy warning if unsigned integer is negative or exceeds the max number for defined type.

image

DSpeichert
DSpeichert previously approved these changes Oct 24, 2020
@DSpeichert
Copy link
Member

DSpeichert commented Oct 28, 2020

Note the potential performance impact as mentioned by Mark.

@MillhioreBT
Copy link
Contributor

This implementation causes performance to be compromised, in fact it drops by about 70%~

You can try the following code and you can compare the time, you will notice that with this implementation increases the time dramatically.

time_t now = OTSYS_TIME();
for (int i = 0; i < 1000000000; i++) {
    getNumber<time_t>(L, 2);
}
std::cout << "Test getNumber of LuaInterface: " << std::setprecision(15) << (double)(OTSYS_TIME() - now) << std::endl;

I know that there are never cases in which getNumber is called with this frequency, I don't know if it is worth it, Someone else should do tests, I don't know if the tests I did were enough or well done

@DSpeichert DSpeichert added decisions Debatable/disputable enhancement Increase or improvement in quality, value, or extent labels Feb 10, 2021
@yamaken93
Copy link
Member

@MillhioreBT are you testing with release version all optimizations enabled?

@MillhioreBT
Copy link
Contributor

@MillhioreBT are you testing with release version all optimizations enabled?

Of course, the code is clear to the naked eye, obviously it takes more time, but it wouldn't be a big problem really, since at small scales there doesn't seem to be a much difference.
If calling getNumber(L, 2); 1000000000 times requires 3 seconds, with these modifications it will require approximately 5 seconds, but let's face it, when can this situation occur?

@MillhioreBT
Copy link
Contributor

It would be amazing to have the opinion of @marksamman about this

@nekiro
Copy link
Member Author

nekiro commented Jul 29, 2021

Is it dropping performance in general or is it with invalid value only?
Well IMO TFS should have something called "debug mode" and I'm not talking about debug build, but config.lua switchable variable which would turn on/turn off certain code, this is one of the cases where it would turn it off/on, cause this can impact performance, but it's very helpful feature.

Copy link
Contributor

@Zbizu Zbizu left a comment

Choose a reason for hiding this comment

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

imo not worth losing 70% of the performance for that. Should be optional in compiler settings.

@DSpeichert
Copy link
Member

The test conducted by @MillhioreBT is synthetic, not representative of what datapacks in the wild do.
In the absence of other comments over the time, I'm willing to take it as a feature unless real-world performance problems are reported.

@DSpeichert DSpeichert merged commit 04d687a into otland:master Feb 6, 2022
@DSpeichert DSpeichert mentioned this pull request Feb 6, 2022
3 tasks
DSpeichert added a commit to DSpeichert/forgottenserver that referenced this pull request Feb 6, 2022
@nekiro nekiro deleted the additional-unsigned-checks branch February 6, 2022 17:44
DSpeichert added a commit that referenced this pull request Feb 6, 2022
@dex-89
Copy link

dex-89 commented Feb 12, 2022

image

After that I have this problem...

DSpeichert added a commit that referenced this pull request Feb 14, 2022
Follow-up to #3926 and #3197.

Co-authored-by: Daniel Speichert <daniel@speichert.pl>
Znote pushed a commit to Znote/forgottenserver that referenced this pull request Feb 23, 2022
Znote pushed a commit to Znote/forgottenserver that referenced this pull request Feb 23, 2022
Znote pushed a commit to Znote/forgottenserver that referenced this pull request Feb 23, 2022
Follow-up to otland#3926 and otland#3197.

Co-authored-by: Daniel Speichert <daniel@speichert.pl>
DSpeichert added a commit that referenced this pull request May 9, 2022
DSpeichert added a commit that referenced this pull request May 9, 2022
Follow-up to #3926 and #3197.

Co-authored-by: Daniel Speichert <daniel@speichert.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decisions Debatable/disputable enhancement Increase or improvement in quality, value, or extent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants