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

Replace ndpi_strnstr() implementation with an optimal one #2447

Merged
merged 4 commits into from
May 22, 2024

Conversation

0xA50C1A1
Copy link
Contributor

@0xA50C1A1 0xA50C1A1 commented May 17, 2024

Please sign (check) the below before submitting the Pull Request:

#2433 #2439

Describe changes:

I apologize for the buggy implementation I suggested last time. This time, I simply took the already quite optimal ndpi_memmem() code and reworked it to handle null-terminated strings. This implementation shows better results in the benchmark, even with the strlen and strnlen calls inside. I've thoroughly tested it, and your unit tests are passing correctly.

Main changes:

  • Efficient substring search: The new implementation utilizes memchr to quickly locate the first character of the needle substring within the haystack. This avoids unnecessary character comparisons, leading to better performance, especially for long strings.
  • Early exit: If the remainder of haystack is shorter than needle, then there's no point in doing unnecessary comparisons.
  • Single loop: The new version uses a single loop to search for the needle

@IvanNardi @utoni what do you think, guys?

@0xA50C1A1 0xA50C1A1 changed the title Optimize performance of ndpi_strnstr() Replace ndpi_strnstr() implementation with an optimal one May 17, 2024
@0xA50C1A1 0xA50C1A1 force-pushed the strnstr branch 2 times, most recently from 03d16ff to eeb4569 Compare May 19, 2024 02:39
@utoni
Copy link
Collaborator

utoni commented May 21, 2024

@0xA50C1A1
Did you do any performance tests again?
Can you please rebase to the current dev branch?

@0xA50C1A1
Copy link
Contributor Author

@0xA50C1A1 Did you do any performance tests again? Can you please rebase to the current dev branch?

Yea, I did. At worst the performance is on par with the old implementation, at best it can be 8 times faster.

@0xA50C1A1 0xA50C1A1 marked this pull request as draft May 21, 2024 12:49
@0xA50C1A1 0xA50C1A1 marked this pull request as ready for review May 21, 2024 14:28
@0xA50C1A1
Copy link
Contributor Author

@utoni @IvanNardi

Worst case:

Test case - Haystack length: 256, Needle length: 20
Average time for ndpi_strnstr: 23.7631 ns
Average time for ndpi_strnstr_opt: 24.0066 ns
ndpi_strnstr is 1.01025 times faster than ndpi_strnstr_opt

Best case:

Test case - Haystack length: 1088, Needle length: 25
Average time for ndpi_strnstr: 317.312 ns
Average time for ndpi_strnstr_opt: 36.6706 ns
ndpi_strnstr_opt is 8.65305 times faster than ndpi_strnstr

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Collaborator

@IvanNardi IvanNardi left a comment

Choose a reason for hiding this comment

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

Thank you!

@IvanNardi IvanNardi merged commit 1564354 into ntop:dev May 22, 2024
36 checks passed
@0xA50C1A1 0xA50C1A1 deleted the strnstr branch May 22, 2024 12:25
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