-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Avoid two hot String allocations in deserialization #17221
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
Signed-off-by: William Woodruff <[email protected]>
|
Note: I realized after-the-fact that this could be subtly incorrect. For example, sometimes serde can't borrow a string because it needs to de-escape it from its JSON or other serialized form. In practice that's probably fine in this context, since we don't really expect any weird escaping to happen on these key names. But it means that we can't just go around swapping Ref: serde-rs/serde#1413 Ref: https://users.rust-lang.org/t/solved-serde-deserialize-str-containig-special-chars/27218 |
|
Can we use |
## Summary Minor, noticed this with #17221. CodSpeed has deprecated `instrumentation` and replaced it with `simulation`, which has the same meaning: https://codspeed.io/docs/instruments/cpu/overview#legacy-terminology ## Test Plan No functional changes. --------- Signed-off-by: William Woodruff <[email protected]>
Replace `String` with `Cow<'_, str>` when deserializing map keys. This allows serde to borrow strings directly from the input in the common case (no escaping needed), while still correctly handling cases where de-escaping is required. Addresses feedback from astral-sh#17221.
Yeah, that would be the most correct/general solution! I think we could probably do the same for every callsite of For example, |
Replace `String` with `Cow<'_, str>` when deserializing map keys. This allows serde to borrow strings directly from the input in the common case (no escaping needed), while still correctly handling cases where de-escaping is required. Addresses feedback from astral-sh#17221.
Summary
Was reading this code while looking at adding PEP 792 support and noticed these: by using the borrowing deserializer instead of the owning one, we can probably save a good number of small-ish String allocations in the
PypiFileandPyxFiledeserializers.(These deserializers in turn are probably pretty hot, since large installations/resolutions require pulling lots of candidates from indices.)
Test Plan
No functional change expected; I'm not sure what the best way to microbenchmark this is. I'll try and contrive something locally.