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

removing bounds checking #19

Merged
merged 1 commit into from
Dec 31, 2023

Conversation

fabi321
Copy link
Contributor

@fabi321 fabi321 commented Dec 28, 2023

This nets me 2.5% - 6% improvement depending on the benchmark

@fabi321
Copy link
Contributor Author

fabi321 commented Dec 28, 2023

Another option would be to just supply pointers.

@sbernauer
Copy link
Owner

Hi @fabi321 thanks for raising this! I actually started a rewrite a few months ago. but did not manage to complete it yet.
You can find it here. It is mostly a refactoring, but also has some small improvements such as having a Trait for Parsers and playing around with native x86 assembler code for the parser.

Especially out of interest is sbernauer/breakwater-rewrite-2@512e07c, where I did basically the same thing you did here. I even did go a step further and - as you mentioned - used raw pointers. I think this way we can avoid further bound checks in simd_unhex (I noticed this in the first place by looking at the assembly of simd_unhex)

As I only have this crappy overheating Laptop and you have your awesome fixed CPU-frequency script, would you be willing to give sbernauer/breakwater-rewrite-2@512e07c a try? (Or in general the whole https://github.com/sbernauer/breakwater-rewrite-2 project if possible if you are interested). In general if you are interested in picking up breakwater-rewrite-2 please say so :)

All the best!

@sbernauer
Copy link
Owner

I think it doesn't matter too much if we use pointers or slices here, so LGTM 👍

@sbernauer sbernauer merged commit 58bae17 into sbernauer:master Dec 31, 2023
7 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.

None yet

2 participants