Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
src/cargo/core/resolver/context.rs
Outdated
There was a problem hiding this comment.
If Borrow is implemented for InternedString would it allow lookups with &str here perhaps?
There was a problem hiding this comment.
impl Borrow<str> for InternedString Hmmm. We are currently Hashing InternedString and str the same. So we could. I have a vain hope that we can find some way to have O(1) hashing. but we don't now. So we can always remove Borrow if we gain faster hashing.
Another peace is to make a singleton for each constant string we interne, it is mostly just "default".
There was a problem hiding this comment.
Yeah I think for now having Borrow may be the best bet in that it'll hopefully give us a nice ergonomic boost in cases like this and also avoid unnecessary temporary allocations in theory
There was a problem hiding this comment.
Added, and it is an ergonomic boost. It should also save one call to hash and 2 cash misses. InternedString::new only allocates if the value is unseen, so this was not really allocating in the hot path. But optimizers are odd so maybe removing the unused branch will help.
src/cargo/sources/registry/mod.rs
Outdated
There was a problem hiding this comment.
I think for these the Cow was due to the derive above to handle escapes in names if they ever creep in
There was a problem hiding this comment.
So do you want me to leave the cows in, incase they are useful; or, do you want me to take them out and we can re-add them if needed?
Currently we are scrubbing data by reing the raw version from RegistryDependency then cleaning it into a local and using the local instead.
|
Ah... like if the name has a " in it. I will put the cows back in the
morning and see if I can add a test to keep this supported.
|
63b59da to
ea957da
Compare
|
Cows are back, and test is added. |
|
@bors: r+ 👍 |
|
📌 Commit f4ef416 has been approved by |
More interned string This is a series of attempts to remove temporary strings. There is no evidence that this will help with anything, but it just seems cleaner. Probably easier to read one commit at a time.
|
☀️ Test successful - status-appveyor, status-travis |
|
I was just looking at some profiles from ~90% was checking if files exist, reading files, or asking git to do the same. |
|
Nice! How long does that take locally for you as well? 90% checking for files and such is pretty extreme, and it may be the next scaling bottleneck for Cargo... |
|
~6 sec. I am a windows based lifeform and my AV may be uppity about large amounts of fs activity. |
|
Oh jeez yeah, 6s just to generate a lockfile is pretty huge! That's a cost that's always paid on startup for Cargo :( |
|
I think it is not so long for evry cargo command. I don't wait that long for other projects. I think it is just that servo has a lot of git dependencies specified. Eche one needs to be checked out and parsed and checked for being a workspace, each time it is updated. So I don't think that a servo>cargo +nightly generate-lockfile -Z no-index-update
Updating git repository `https://github.com/servo/osmesa-src`
Updating git repository `https://github.com/servo/webrender`
Updating git repository `https://github.com/servo/gaol`
Updating git repository `https://github.com/servo/devices`
Updating git repository `https://github.com/energymon/energymon-rust.git`
Updating git repository `https://github.com/pcwalton/signpost.git`
Updating git repository `https://github.com/servo/rust-azure`
Updating git repository `https://github.com/servo/fontsan`
Updating git repository `https://github.com/energymon/energymon-sys.git` |
|
Ah ok that makes more sense! |
This is a series of attempts to remove temporary strings. There is no evidence that this will help with anything, but it just seems cleaner.
Probably easier to read one commit at a time.