Evaluating ROI of codebase-wide reduction of std::string copies
Jeffrey Yu
Hi folks,
I am writing to solicit community feedback on a refactoring effort I have been running: http://issues.chromium.org/issues/501076278.
The project aims to reduce redundant string copies across the codebase, inspired by a headline I read years ago that `std::string` was responsible for almost half the RAM allocation in the Chrome browser (this stat probably isn't true anymore). While Small String Optimization (SSO) has mitigated the cost of allocating small strings, I believe there are still meaningful performance gains to be realized - specifically for the Chromium browser, which often runs on low-end hardware ($200 Chromebooks and $50 tablets) where memory and CPU overhead are significant for the end-user.
I am aware that even "volunteer" CLs incur costs as outlined in the Rubric for evaluating cost/benefit of CLs:
Reviewer cost
Increased risk of regressions
Noiser Git blame
Increased time to resolve merge conflicts
Changing established and well-known code patterns
I am keen to hear whether you believe this work aligns with current Chromium goals, or if there are specific concerns regarding these patterns that I should be aware of. If the community generally feels the cost-benefit ratio is not there, I am happy to pause these efforts.
If discussion of the merits of individual changes is warranted, we can make different decisions on some of the individual workstreams, and prioritize or discourage them depending on ROI.
Here are some patterns I've been employing, along with some starting discussion. (Example CL: https://crrev.com/c/7797391)
Use `std::move` on the last usage of strings (or containers) to avoid a copy
Readability: Standard C++ usage, not limited to strings.
Safety: Can result in errors if moving a string still in use, but compiler or presubmit checks can often catch many such cases.
Use utility functions like `base::StrCat` and `base::StrAppend` for concatenation instead of the repeated concatenation (`result = x + y + z`) which creates temporary string copies
Readability: Somewhat more verbose, but also a commonly understood pattern.
Safety: None that I'm aware of.
Use `std::string_view` instead of string to avoid making copies of string literals.
Change function param from `const std::string&` to `std::string_view` so that string literals passed to it don't create a copy
Change `std::string = "some string"; to `std::string_view = some_string" to avoid a copy.
Functions that only return string literals can have return type changed from string to string_view to avoid making a copy on every function call.
Instead of converting characters to a string (`std:string(kPrefix);`) use a string_view instead (`std::string_view(&kPrefix, 1);`) to avoid a copy.
Readability: Mostly the same, slightly decreased in char-to-view conversion.
Safety: None, for compile-time string literals or conversion of const ref string params.
Use `std::string_view` instead of a string copy for temporary copies that don't need to outlive the parent string
Readability: Unaffected, mostly the same, unless the string_view is created inline (`function(std::string_view(original_str), param_b, param_c)`.
Safety: Can lead to safety issues if a string_view is mistakenly assigned a temporary string, or a non-const string that changes value.
Refactor use of `std::string::subtr` to avoid string copies.
Call `.substr` on string_view instead of a string to get a string to get another string_view instead of a copy.
[C++23] Call `std::move(str).substr()` to avoid a copy.
If modifying a string, use `std::string::erase` instead of `substr` to modify the string in-place instead of making a copy.
Similar refactors with string_view based modifications using `std::string_view::remove_prefix`, `std::string_view::remove_suffix`, etc instead of `std::string::erase` or other edit operations
Readability: Wrapping strings in `std::string_view` or `std::move` can minorly affect readability
Safety: SWEs using string_view or move need to be aware of lifetime issues.
Use the string_view version of `base::TrimWhitespace` instead of the 3-argument version that assigns to a string.
Readability: Unaffected.
Safety: Similar to other string_view-related lifetime issues as above.
Use `base::SplitStringPiece` instead of `base::SplitString` to avoid making copies
Readability: Unaffected.
Safety: Similar to other string_view-related lifetime issues as above.
Use methods that return string_views instead of string copies, e.g. `GURL::content()`, `GURL::scheme()`, `GURL::host()` instead of `GURL::GetContent()`, `GURL::GetScheme()`, `GURL::GetHost()`, etc.
Readability: Unaffected.
Safety: Similar to other string_view-related lifetime issues as above.
Thanks for your time and guidance.
(This is also published as a commentable doc: https://docs.google.com/document/d/1bqCLZP_xCuecp85P6WLAOPy2NbNBEYNCP4_0HqivdCc/edit?resourcekey=0-oM8jfYv5M71ffKDWUa-cnQ&tab=t.0)
Colin Blundell
- The changes that you've landed so far are good to have in the codebase - thanks!
- It would be good (if we don't have this already) to capture the items that you've highlighted somewhere in the codebase as "best practices" for new code (be sure to include folks such as +Marco Vitale who commented on them to ensure that there's full alignment).
- WRT further investments of your time doing automated refactors in the codebase - it sounds like we can work together to find higher-impact efforts for you to contribute to, e.g. spanification, DCHECK->CHECK conversion in security-critical code, and/or dead base::Feature removal.
Hi,Without commenting on the cost per se, from the R of "ROI" I would suggest to evaluate the in-the-wild potential upside with:
- Memory: We have memory profiling, at least for some processes. Googlers can go to go/flamegraphs, and get this one for instance. It looks like for the browser process, we are indeed using a lot of memory for strings, but to my eyes the largest consumers are unlikely to suffer from duplication due to unwanted copies in C++ (duplication could be at a higher level. for instance an extension that has many strings with identical content)
- CPU: Similarly, using the same link to get an estimate of CPU time spent in string copies. I haven't run the query yet though.
All in all, I believe (but don't quote me on that for the CPU side) that we are not seeing too much cost coming from string copies in the field. However, it is good hygiene for new code to use the patterns above: we certainly had cases in the past that led to surprising issues (for instance, in the distant past, when we migrated from a STL that did copy on write to one that didn't, we had... interesting surprises).I will let others comment on the non-performance aspects of the calculation.--Benoit
--
You received this message because you are subscribed to the Google Groups "Chrome Technical Steering Council" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chrome-tsc+...@google.com.
To view this discussion visit https://groups.google.com/a/google.com/d/msgid/chrome-tsc/CAD4jpnMANkqMSx%3Dibka5yh6bjUAQc%2B-hS%2B0jvrg5i4THwCuxAw%40mail.gmail.com.
Benoit Lize
- Memory: We have memory profiling, at least for some processes. Googlers can go to go/flamegraphs, and get this one for instance. It looks like for the browser process, we are indeed using a lot of memory for strings, but to my eyes the largest consumers are unlikely to suffer from duplication due to unwanted copies in C++ (duplication could be at a higher level. for instance an extension that has many strings with identical content)
- CPU: Similarly, using the same link to get an estimate of CPU time spent in string copies. I haven't run the query yet though.
Kyle Charbonneau
Andrew Grieve
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAERSRDAyJ%2B5Gc3%3De3oH17JbakPpXeV%2BNFuWvr2uHF2f-xp9czg%40mail.gmail.com.
Nico Weber
I haven't heard any opposition that is against the churn refactor CLs cause... In terms of ROI, one thing that matters a lot is the "I". Seems like the wins are not nothing if it shows at all in a profile, and we want these patterns for new code, so if the time is also very small, then may very well be a good ROI!
Jeffrey Yu
Since I started the thread, I thought I'd chime in also on my thoughts about ROI:
Return:
Codebase reflects best practices: I see this as the biggest ROI from refactoring. Both humans and AI pattern-match their code output to existing code, and having our code reflect best practices is an investment in the future code quality as well. This is why I took the "refactor everywhere" approach rather than trying to optimize for certain paths (and also why I sometimes refactored even test-only code, though at a lower priority).
Improving the product: While the performance gains are small, they're still a net gain, and many volunteer/community contribution efforts at Google often target low-cost, high-readability changes, because they're easy to contribute to and also make minor improvements at scale.
For "at scale", the benefits work differently in Google-internal (long-running server processes) vs in Chromium (widely-used consumer product). For internal projects, the primary benefit would be from Google saving significant compute cycles. For Chromium, the benefits would primarily be distributed among users, but would still be significant.
Extreme example: In 2014, the Omnibox allocated 25K `std::string` instances per keystroke. While that number has improved over time, opportunities still exist to reduce string allocations (see recent examples from the current effort: https://crrev.com/c/7796145, https://crrev.com/c/7797391)
Since Chrome has billions of users, a feature like the Omnibox having trillions of keystrokes per day can generate significant savings if improved:
3 billion users x 100 keypresses / day = 300 billion keypresses / day
Even saving 10 string copies in Omnibox would result in 3 trillion string allocations saved daily. A quick query to Gemini says this saves roughly 8.33 kWh of electricity and 3.33 kg of CO2 without SSO, or roughly 2.33 kWh and 0.93 kg of CO2 with SSO enabled, from a single CL.
Cost:
Readability: The types of refactoring chosen were typically taken from best practices guides published by Google both internally and externally (such as https://abseil.io/tips/), so the intention is to not affect readability too much. The CLs try to refactor to standard idioms that are well-known (or should be). There's a number of different refactors, and the different impact on readability of each can be discussed separately, but many refactors often have drop-in replacements (e.g. string->string_view, x + y -> StrCat(x, y), SplitString->SplitStringPiece.
Code review: This is probably the biggest cost, especially since I don't have committer status and need 2 stamps, and this requirement will probably never go away unless we can trust AI reviewers to a high degree. While a fixed, one-time cost per CL, SWE-hours do cost relatively high compared to other resources. Potential solutions:
A process that allows cleanup/refactoring CLs be redirected to special cleanup review queues, staffed by volunteer reviewers willing to take the review burden, and could potentially review larger CL batch sizes, and with global permissions to approve trivial changes, similar to the LSC process.
AI-powered review that can approve and stamp certain types of CLs. Maybe if CL delta falls below a certain size threshold, and below a certain level of complexity measured by a code complexity analyzer tool, those CLs can be approved by an agent.
Blame fragmentation
This is listed as another cost, although I'm not sure there's a good metric that shows how much this actually costs in measurable units. Potentially, though, this could be mitigated with use of AI to help explain code and search through CL history to understand intention and authors behind implementation.
Daniel Cheng
Colin Blundell
- Opportunity cost on author and reviewers (even if AI/tooling is generating the CLs, shepherding any effort like this at Chromium scale has a significant carrying cost on both author and reviewers, both of whom have a finite amount of time available)
- Risk of regressions (potentially smaller if CLs are generated entirely by deterministic tooling)