-
Notifications
You must be signed in to change notification settings - Fork 209
Worked on vector-related performance #692
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
Conversation
Kixiron
commented
Apr 2, 2020
- Pre-allocated vectors wherever possible in order to reduce the total amount of allocations preformed
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.
Most of these would be better written as map
s IMO.
Changed the vast majority to maps, some really didn't lend to it |
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.
Lots of comments! Most of them are just nit-picky things, I'm happy to have them addressed in a follow-up PR instead.
Thanks so much for working on this! Little things like this are easy to forget about but they make a big difference :) |
I would like to add tests for these before merging. See e.g. https://github.com/rust-lang/docs.rs/blob/master/src/web/mod.rs#L617 for examples of how to write tests, and you can visit the pages on docs.rs to see what the expected behavior should be. I know there's a lot of changes, it might be easier to add tests in a separate PR and merge that first. |
Git hates me and closed on its own |
Status: this needs to be rebased and needs tests to be written. |
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 was a little picky on the last review. This looks good once you rebase and address the outstanding comments (I resolved a lot of them). db/file.rs
changed a lot in #643 so you might want to make those changes in a follow-up PR instead of trying to rebase.
@jyn514 Fixed |
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.
Thanks for all the hard work! Unfortunately I took long enough to review that there are merge conflicts. Once those are resolved (preferably through rebasing, not merging) this looks great.
4d00880
to
b50f3f7
Compare
🎉 🎉 |
This was a regression introduced in rust-lang#692.
This was a regression introduced in #692.