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

Fix BufferBackend soundness issue and add StringInterner::resolve_unchecked #68

Merged
merged 2 commits into from
May 1, 2024

Conversation

Robbepop
Copy link
Owner

@Robbepop Robbepop commented May 1, 2024

Closes #49.

This fix vastly deteriorates the performance of BufferBackend::resolve which is why I added StringInterner::resolve_unchecked for users to mitigate the performance issue on their side if they are sure to always use valid symbols.
Even with the regressed performance for resolve the BufferBackend is the choice for especially memory constrained environments.

Benchmarks:

resolve/already-filled/BucketBackend
                        time:   [61.173 µs 61.271 µs 61.368 µs]
                        thrpt:  [1.6295 Gelem/s 1.6321 Gelem/s 1.6347 Gelem/s]

resolve/already-filled/StringBackend
                        time:   [78.175 µs 78.260 µs 78.359 µs]
                        thrpt:  [1.2762 Gelem/s 1.2778 Gelem/s 1.2792 Gelem/s]

resolve/already-filled/BufferBackend
                        time:   [448.82 µs 449.71 µs 450.60 µs]
                        thrpt:  [221.93 Melem/s 222.37 Melem/s 222.81 Melem/s]

resolve_unchecked/already-filled/BucketBackend
                        time:   [59.920 µs 59.999 µs 60.081 µs]
                        thrpt:  [1.6644 Gelem/s 1.6667 Gelem/s 1.6689 Gelem/s]

resolve_unchecked/already-filled/StringBackend
                        time:   [72.390 µs 72.483 µs 72.589 µs]
                        thrpt:  [1.3776 Gelem/s 1.3796 Gelem/s 1.3814 Gelem/s]

resolve_unchecked/already-filled/BufferBackend
                        time:   [58.920 µs 58.995 µs 59.078 µs]
                        thrpt:  [1.6927 Gelem/s 1.6950 Gelem/s 1.6972 Gelem/s]

Unfortunately this fix vastly regresses the performance of the method.
Benchmarks show -73% throughput which is massive ...

Still the BufferBackend is a viable choice for memory constrained environments.
We added this because it make a huge difference for the BufferBackend to have this available.
@Robbepop Robbepop merged commit 3dbdfe1 into master May 1, 2024
15 of 16 checks passed
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.

BufferBackend: internal method has unsound signature
1 participant