search

goatshriek / stumpless

a C logging library built for high performance and a rich feature set
https://goatshriek.github.io/stumpless
Apache License 2.0
443 stars 321 forks source link

Refactored prival from string for performance #437

Closed Griezn closed 1 week ago

Griezn commented 3 weeks ago

This PR addresses issue #428 by significantly reducing allocations and execution time, as demonstrated by the run-performance-test-prival results.

Updated get_x_from_buffer functions to correctly utilize the length parameter and ensure function test correctness.

The check I removed failed. I don't know exactly why it the check didn't exist in the corresponding test of facility so I removed it.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.48%. Comparing base (fb0d2e0) to head (f894f7f). Report is 1 commits behind head on latest.

Files with missing lines Patch % Lines
src/strhelper.c 66.66% 1 Missing and 2 partials :warning:
src/severity.c 75.00% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## latest #437 +/- ## ========================================== - Coverage 90.86% 90.48% -0.38% ========================================== Files 47 47 Lines 4389 4373 -16 Branches 586 585 -1 ========================================== - Hits 3988 3957 -31 - Misses 269 281 +12 - Partials 132 135 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

goatshriek commented 3 weeks ago

Just one more header fixup and I believe this will be ready.

Griezn commented 2 weeks ago

So what I did now is, I solved issue #438 also with this pr. With this fix there are no allocations performed by the function so from my perspective those tests become irrelevant. This also allowed me to remove the check I added to see if there already was an error.

But I think the issue of errors being overwritten can still occur in other places so maybe this is a new issue.

goatshriek commented 2 weeks ago

The Windows builds are failing because strncasecmp isn't completely portable, it seems. I can think of two ways forward right now. The first is a bit simpler: to add a custom implementation of strncasecmp to the strhelper files (probably with a different function name) and use this. The second is more involved but will be the end state of the project: add a configuration step to detect if the build environment has a library function that can be used (strncasecmp, _strnicmp, etc) and uses this if it is available, and otherwise falls back to an embedded default.

Since the second option will require some cmake work and the addition of several more source files and config_ functions (see the portability docs for more info), I would just stick with the first option in this change. I can create a second issue to track the second part of this, since using the system library would reduce library size and potentially improve performance when available.

goatshriek commented 1 week ago

Thanks once again for putting this change together, and sticking with it through all of the requested changes!