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

Use starts_with() instead of substr(0, N) getting and comparing to prefix #3702

Merged
merged 5 commits into from
Aug 15, 2022

Conversation

DmitryAk
Copy link
Contributor

@DmitryAk DmitryAk commented Aug 13, 2022

Issue

There were several places in code such as:

else if (tag_.first.substr(0, 20) == "motorcar:conditional" ||

It seems not very reliable and not very performant:

  1. It is required to specify the exact prefix size in substr(0, N). If there is a mistake - the result of comparing the substr with prefix will always be false. It looks not very convenient and not very reliable.
  2. It always creates an auxiliary substring, and this substring is compared to the prefix (so even in best case this requires O(N), where N is prefix size). At the same time it is possible that starts_with can just compare the first chars - and reject further comparing (so in best case this requires O(1)). Also starts_with doesn't require unnecessary auxiliary substring (so no unnecessary allocation is guaranteed). To check this, a benchmark test was introduced in this PR, results (Release configuration, x86_64, clang-1316.0.21.2.5):
With substr() it took 8224 ms
With starts_with() it took 1963 ms

Tasklist

@kevinkreiser
Copy link
Member

Looks good to me just waiting for the build to pass

@kevinkreiser kevinkreiser merged commit 736c594 into valhalla:master Aug 15, 2022
@DmitryAk DmitryAk deleted the da-use-starts_with branch August 15, 2022 05:39
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