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

Severe performance regression in 1.8.6 #294

Closed
joankaradimov opened this issue Mar 29, 2024 · 8 comments
Closed

Severe performance regression in 1.8.6 #294

joankaradimov opened this issue Mar 29, 2024 · 8 comments

Comments

@joankaradimov
Copy link

joankaradimov commented Mar 29, 2024

I'm running this grammar:
https://github.com/joankaradimov/Magnetar/blob/master/MagnetarCraft/src/iscript_parser.cpp#L71
... with this input file:
https://raw.githubusercontent.com/neivv/aice/master/test_scripts/vanilla.txt

On v1.8.5 I'm observing parse times of below 1 second. On 1.8.6 it's above 10 seconds.

Similarly -- on the web playgound I was running identical examples for under 150 ms the previous week. I'm assuming the version over there was updated these last couple of days, since the execution times are suddenly 1-2 orders of magnitude worse.

@yhirose
Copy link
Owner

yhirose commented Mar 29, 2024

@joankaradimov I guess this commit causes this problem. I'll take a look at it some time this weekend. Thanks for catching the serious problem!

@joankaradimov
Copy link
Author

joankaradimov commented Mar 30, 2024

In that case I think it might be this line. There are multiple std::string constructors being called within a while loop. This line can easily be moved outside the loop. And then the additional std::string allocations can be avoided altogether in the non-case-insensitive case.

This improves the situation a little bit, but it does not resolve it completely. The performance is still worse than 1.8.5.

A much better solution would be to get rid of the ignore_case_ checks in match and tweak the dictionary in the trie...

std::map<std::string, Info, std::less<>> dic_;

... to use case-insensitive comparisons instead of std::less when necessary.

@yhirose
Copy link
Owner

yhirose commented Mar 31, 2024

@joankaradimov could you check performance with the latest peglib.h in the master branch? If you confirm that it fixes the performance regression, I'll bump the version to 1.8.8. Thanks!

@joankaradimov
Copy link
Author

@yhirose I can confirm the problem is fixed.

From ~10 seconds previously down to below 50 ms now.

@yhirose
Copy link
Owner

yhirose commented Apr 1, 2024

It's now included in v1.8.8. Thanks for your fine contribution!

@joankaradimov
Copy link
Author

Thank you for the quick responses and fixes!

@davemcatcisco
Copy link

@yhirose - Thanks for this. Are you responsible for the vcpkg? That's still sitting at 1.8.6.

@yhirose
Copy link
Owner

yhirose commented Apr 6, 2024

@joankaradimov no. I don't know who are working on vcpkg, or other package managers like conan.

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 a pull request may close this issue.

3 participants