-
Notifications
You must be signed in to change notification settings - Fork 141
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
Deduplicate findIndexOrEnd by exporting it from Data.ByteString.Internal #337
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to other suggestions for the section name "Internal indexing".
Just "Indexing" would also do, IMHO.
Does this PR improve any benchmark results? Feel free to add benchmarks if the current suite doesn't cover the issue from #334.
There are no existing benchmarks that test the affected functions as far as I'm aware. I could add a new one in this PR, but then we would only see the new "score". The affected functions are the functions in I think So, I am now thinking of adding extra benchmarks in The Also, I don't know what the best input would be for these benchmarks. For |
|
@noughtmare sorry to nudge, but I'd like to get this merged before the next release. |
Sorry for the long wait. I'm thinking of reordering the history so that the benchmarks are added before the changes. That way, you can more easily benchmark before and after the changes. I'm also having trouble with naming things. |
I've added some benchmarks. The results on my machine:
No spectacular changes, but almost everything improves a little. I think group zero-one is not all that much different, but I'm confused by the regression of |
On my machine benchmarks are:
vs.
So performance improves all across the board, and I do not observe any regression for |
It occurred to me that these benchmarks will need to be changed if rewrite rules are added that would rewrite |
@noughtmare yes, this is a good idea, let's replace |
I have changed the benchmarks. |
Thanks! |
Closes #334
I am open to other suggestions for the section name "Internal indexing".