Skip to content

Added checks around trick - #2

Closed
gilescope wants to merge 9 commits into
pickfire:masterfrom
gilescope:master
Closed

Added checks around trick#2
gilescope wants to merge 9 commits into
pickfire:masterfrom
gilescope:master

Conversation

@gilescope

Copy link
Copy Markdown

Added all the std lib checks around trick and the time seems to double for me from 4ns to 8ns for a signed 64.

Keen to get your feedback.

@gilescope

Copy link
Copy Markdown
Author

Hmm, feel a little uncomfortable with how that copy_nonoverlapping is set up when the string isn't big enough. Let me revise that with something less dubious.

This shifts that memory out and also slightly faster so now small u64 with checks is same speed as trick.
@gilescope

Copy link
Copy Markdown
Author

Ok, I think this version is sound. It is still copying a little bit of memory that's unknown which I don't have a feel for if that's really bad or just not good? Maybe we can tweak copy_nonoverlapping to only copy the str length.

@gilescope

Copy link
Copy Markdown
Author

And in the morning light I see that 16 chars is not long enough for u64, need 20.

@pickfire

pickfire commented Apr 4, 2021

Copy link
Copy Markdown
Owner

Thanks for sending in the pull request but I don't think str parsing is for this repository. Because in this repository it's specific to parsing 16 characters and the input is expected to be 16 characters long.

But if you still want, can submit a version with checks as well but this still targets 16 characters. What I am suggesting in the rust pull request is that this repository could be a reference to test out the performance of a faster str parsing, so that we can easily benchmark it. Note that we may also want to turn off target-cpu=native because rust compiler by default is not built for native cpu.

Closing this for now but if you think it should be here then please tell me.

@pickfirepickfire closed this Apr 4, 2021
@gilescope

Copy link
Copy Markdown
Author

Sorry - I should have realised that you were aiming specifically for 16 digits in this repo. I think maybe the best route forwards is for me to close down the rust radix10 pr and just make a atoi_radix10 specific crate based around trick and meanwhile push forward with the other rust PR that speeds up all radix for smaller numbers.

@pickfire

Copy link
Copy Markdown
Owner

Yes, that is the best way, don't need to make it a crate can just make a repo to experiment with it. Can also try to benchmark and see if it affects smaller and larger integers.

Sign up for freeto 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

@gilescope@pickfire