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

kaizen: Increase throughput with flexible FA traversal #332

Merged
merged 2 commits into from
Jul 12, 2024
Merged

Conversation

timbray
Copy link
Owner

@timbray timbray commented Jul 10, 2024

We took a ~20% performance penalty with the introduction of NFAs (and thus full shellstyle support) and this is shown up particularly in numeric patterns. It turns out that while our data structure is designed to support NFAs, a lot of the FAs we generate are actually deterministic, i.e the combination of a state and a byte always causes a transfer to zero or one other state. Thus it is possible to traverse the FA without keeping track of multiple current/next states, vastly reducing the amount of memory management required. This PR led to ~20% performance improvement on common cases, no change on shellstyle, probably >20% on numerically-heavy patterns. 20% is pretty good since half our CPU/elapsed time still goes into flattening the events.

@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

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

Project coverage is 96.56%. Comparing base (c28897d) to head (b151669).

Files Patch % Lines
value_matcher.go 75.00% 1 Missing and 2 partials ⚠️
small_table.go 87.50% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #332      +/-   ##
==========================================
- Coverage   96.73%   96.56%   -0.17%     
==========================================
  Files          18       18              
  Lines        1837     1864      +27     
==========================================
+ Hits         1777     1800      +23     
- Misses         34       36       +2     
- Partials       26       28       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@timbray
Copy link
Owner Author

timbray commented Jul 11, 2024

OK, so I have verified that the benchmark in question, Benchmark_JsonFlattner_Evaluate_ContextFields runs considerably faster as a result of this PR, compared to the previous PR. Looking at the Ci job benchmarks.yml I get the idea that if the comparison fails it refuses to store the output for comparison with subsequent results. Hey, @embano1 and @yosiat, I think one of you set this up. Any suggestions what a good way is to reset the baseline to the value from the last PR, or this one? In the meantime I will probably merge this PR because efficiency is good.

@timbray
Copy link
Owner Author

timbray commented Jul 12, 2024

I set fail-on-alert: false in the benchmarks.yml workflow to allow this to succeed, and presumably to reset the cache. If this works, I will set it back to true after pushing this PR. I'm OK with this bit of painful extra work to allow a change that has a performance regression effect, after all they should be very rare.

@timbray timbray merged commit daef7fd into main Jul 12, 2024
7 checks passed
@timbray timbray deleted the determinism branch July 13, 2024 15:52
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.

2 participants