[bindings] Account for the memory of blink::String members in unions. [chromium/src : main]
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
- Charts-chartjs: base median = 37.32000000085681 -> patched median = 37.589999999990695
- NewsSite-Next: base median = 80.34500000001863 -> patched median = 80.47500000046567
- NewsSite-Nuxt: base median = 64.46000000089407 -> patched median = 64.66999999918043
- TodoMVC-JavaScript-ES6-Webpack-Complex-DOM: base median = 38.96499999999068 -> patched median = 39.46000000032363
- TodoMVC-React-Redux: base median = 32.72000000020489 -> patched median = 32.84999999995343
- TodoMVC-Vue: base median = 21.80999999996275 -> patched median = 21.859999999776484
- TodoMVC-jQuery: base median = 98.51000000010245 -> patched median = 99.22000000000699
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14eeed53f10000
Related details
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 1 comment![]( "Open in Gerrit")
Please take a look. Thanks.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 1 comment![]( "Open in Gerrit")
Please take a look. Thanks.
Related details
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
+caseq for bind_gen review
lgtm for the general accounting. Note that this certainly moves around GC as already indicated by the pinpoint job. We need to revert if we find that the overall tradeoff on PGO is not good.
Related details
- Andrey Kosyakov
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
I'm not sure I understand the intent here. We're never retaining any of the unions that you've mentioned. In fact, we're rarely retaining bindings-generated unions at all (there are single digit occurrences of that). These unions normally only exist for a short duration of a bindings call. The strings that use the memory wouldn't usually go anywhere and would be part of DOM, while you would deduct the memory used by them in the union destructor. Why don't we rather account them there? Additionally, with WTF::Strings there would potentially be an issue of double-accounting, as they're refcounted. So to me it looks like we're adding some notable overhead to a rather hot code path with no obvious benefits.
Related details
- Francois Pierre Doray
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
I'm not sure I understand the intent here. We're never retaining any of the unions that you've mentioned. In fact, we're rarely retaining bindings-generated unions at all (there are single digit occurrences of that). These unions normally only exist for a short duration of a bindings call. The strings that use the memory wouldn't usually go anywhere and would be part of DOM, while you would deduct the memory used by them in the union destructor. Why don't we rather account them there? Additionally, with WTF::Strings there would potentially be an issue of double-accounting, as they're refcounted. So to me it looks like we're adding some notable overhead to a rather hot code path with no obvious benefits.
Somewhat agree.
If the string is unique and not double accounted then it could be nice to treat it as one object. (We have done so in other areas when useful)
I don't think it will solve the issue because even if it's large enough to make GC more likely, the GC will not get rid of the string (as the object is alive). Even if the GC reclaims other memory it's not synchronous and will likely not help for this call site.
Related details
- Andrey Kosyakov
- Francois Pierre Doray
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 1 comment![]( "Open in Gerrit")
Michael LippautzI'm not sure I understand the intent here. We're never retaining any of the unions that you've mentioned. In fact, we're rarely retaining bindings-generated unions at all (there are single digit occurrences of that). These unions normally only exist for a short duration of a bindings call. The strings that use the memory wouldn't usually go anywhere and would be part of DOM, while you would deduct the memory used by them in the union destructor. Why don't we rather account them there? Additionally, with WTF::Strings there would potentially be an issue of double-accounting, as they're refcounted. So to me it looks like we're adding some notable overhead to a rather hot code path with no obvious benefits.
Somewhat agree.
If the string is unique and not double accounted then it could be nice to treat it as one object. (We have done so in other areas when useful)
I don't think it will solve the issue because even if it's large enough to make GC more likely, the GC will not get rid of the string (as the object is alive). Even if the GC reclaims other memory it's not synchronous and will likely not help for this call site.
"We're never retaining any of the unions that you've mentioned"
Unions are managed objects. Even if we're not retaining them, they're only freed through GC. Accounting for the external memory held by these unions ensures that GC decisions are based on accurate information.
"WTF::Strings there would potentially be an issue of double-accounting, as they're refcounted"
"If the string is unique and not double accounted then it could be nice to treat it as one object"
Yes, double-accounting could occur, if multiple unions acquire a reference to the same string, or if other objects with external memory accounting acquire a reference to the same string. A similar problem exists [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/message_event.cc;l=69;drc=e89a20d491d0946bdc64a17d5461c757b6954f4f) for example. We could solve it through [HasOneRef](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/text/string_impl.h;l=279-281;drc=e89a20d491d0946bdc64a17d5461c757b6954f4f) (i.e. only account for the memory if we're the only owner), or other solutions.
"I don't think it will solve the issue" Data points:
- This unaccounted memory is involved in 2.4% of all Chrome crashes (not just OOMs - go/ooms-from-text-content-or-outer-html).
- On average, 15% of the malloc'ed memory in renderers with >500MiB of malloc'ed memory is retained by these unions - go/heap-flame-graph-union-memory-feb2026.
- I confirmed that the current solution prevents growing memory usage to multiple GiB and OOM'ing on https://francoisdoray.github.io/web-repros/outer-html-leak.html.
Proposal: Land this and observe impact on OOMs and PMF. Based on observations, revert or refine (e.g. design a proper solution for the double-accounting).
Related details
- Andrey Kosyakov
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
Michael LippautzI'm not sure I understand the intent here. We're never retaining any of the unions that you've mentioned. In fact, we're rarely retaining bindings-generated unions at all (there are single digit occurrences of that). These unions normally only exist for a short duration of a bindings call. The strings that use the memory wouldn't usually go anywhere and would be part of DOM, while you would deduct the memory used by them in the union destructor. Why don't we rather account them there? Additionally, with WTF::Strings there would potentially be an issue of double-accounting, as they're refcounted. So to me it looks like we're adding some notable overhead to a rather hot code path with no obvious benefits.
Francois Pierre DoraySomewhat agree.
If the string is unique and not double accounted then it could be nice to treat it as one object. (We have done so in other areas when useful)
I don't think it will solve the issue because even if it's large enough to make GC more likely, the GC will not get rid of the string (as the object is alive). Even if the GC reclaims other memory it's not synchronous and will likely not help for this call site.
"We're never retaining any of the unions that you've mentioned"
Unions are managed objects. Even if we're not retaining them, they're only freed through GC. Accounting for the external memory held by these unions ensures that GC decisions are based on accurate information."WTF::Strings there would potentially be an issue of double-accounting, as they're refcounted"
"If the string is unique and not double accounted then it could be nice to treat it as one object"
Yes, double-accounting could occur, if multiple unions acquire a reference to the same string, or if other objects with external memory accounting acquire a reference to the same string. A similar problem exists [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/message_event.cc;l=69;drc=e89a20d491d0946bdc64a17d5461c757b6954f4f) for example. We could solve it through [HasOneRef](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/text/string_impl.h;l=279-281;drc=e89a20d491d0946bdc64a17d5461c757b6954f4f) (i.e. only account for the memory if we're the only owner), or other solutions."I don't think it will solve the issue" Data points:
- This unaccounted memory is involved in 2.4% of all Chrome crashes (not just OOMs - go/ooms-from-text-content-or-outer-html).
- On average, 15% of the malloc'ed memory in renderers with >500MiB of malloc'ed memory is retained by these unions - go/heap-flame-graph-union-memory-feb2026.
- I confirmed that the current solution prevents growing memory usage to multiple GiB and OOM'ing on https://francoisdoray.github.io/web-repros/outer-html-leak.html.
Proposal: Land this and observe impact on OOMs and PMF. Based on observations, revert or refine (e.g. design a proper solution for the double-accounting).
Are unions other than those two mentioned a concern? Would it help if we make selected unions non-GC'ed? Since they're never retained, we could introduce an [ExtendedAttribute] to generate a non-GC'ed version of that minimizing heap thrashing somewhat, or, where possible, even replace their usage with overloads?
Related details
- Francois Pierre Doray
- Michael Lippautz
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 1 comment![]( "Open in Gerrit")
Michael LippautzI'm not sure I understand the intent here. We're never retaining any of the unions that you've mentioned. In fact, we're rarely retaining bindings-generated unions at all (there are single digit occurrences of that). These unions normally only exist for a short duration of a bindings call. The strings that use the memory wouldn't usually go anywhere and would be part of DOM, while you would deduct the memory used by them in the union destructor. Why don't we rather account them there? Additionally, with WTF::Strings there would potentially be an issue of double-accounting, as they're refcounted. So to me it looks like we're adding some notable overhead to a rather hot code path with no obvious benefits.
Francois Pierre DoraySomewhat agree.
If the string is unique and not double accounted then it could be nice to treat it as one object. (We have done so in other areas when useful)
I don't think it will solve the issue because even if it's large enough to make GC more likely, the GC will not get rid of the string (as the object is alive). Even if the GC reclaims other memory it's not synchronous and will likely not help for this call site.
Andrey Kosyakov"We're never retaining any of the unions that you've mentioned"
Unions are managed objects. Even if we're not retaining them, they're only freed through GC. Accounting for the external memory held by these unions ensures that GC decisions are based on accurate information."WTF::Strings there would potentially be an issue of double-accounting, as they're refcounted"
"If the string is unique and not double accounted then it could be nice to treat it as one object"
Yes, double-accounting could occur, if multiple unions acquire a reference to the same string, or if other objects with external memory accounting acquire a reference to the same string. A similar problem exists [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/message_event.cc;l=69;drc=e89a20d491d0946bdc64a17d5461c757b6954f4f) for example. We could solve it through [HasOneRef](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/text/string_impl.h;l=279-281;drc=e89a20d491d0946bdc64a17d5461c757b6954f4f) (i.e. only account for the memory if we're the only owner), or other solutions."I don't think it will solve the issue" Data points:
- This unaccounted memory is involved in 2.4% of all Chrome crashes (not just OOMs - go/ooms-from-text-content-or-outer-html).
- On average, 15% of the malloc'ed memory in renderers with >500MiB of malloc'ed memory is retained by these unions - go/heap-flame-graph-union-memory-feb2026.
- I confirmed that the current solution prevents growing memory usage to multiple GiB and OOM'ing on https://francoisdoray.github.io/web-repros/outer-html-leak.html.
Proposal: Land this and observe impact on OOMs and PMF. Based on observations, revert or refine (e.g. design a proper solution for the double-accounting).
Are unions other than those two mentioned a concern? Would it help if we make selected unions non-GC'ed? Since they're never retained, we could introduce an [ExtendedAttribute] to generate a non-GC'ed version of that minimizing heap thrashing somewhat, or, where possible, even replace their usage with overloads?
I’m concerned about the lifetime of strings returned from innerHTML (v8_element::InnerHTMLAttributeGetCallback), outerHTML (v8_element::InnerHTMLAttributeGetCallback), and textContent (v8_node::TextContentAttributeGetCallback). If there’s an alternative approach to ensure these strings aren't retained longer than necessary, I’m definitely open to suggestions!
Related details
- Andrey Kosyakov
- Michael Lippautz
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
Michael LippautzI'm not sure I understand the intent here. We're never retaining any of the unions that you've mentioned. In fact, we're rarely retaining bindings-generated unions at all (there are single digit occurrences of that). These unions normally only exist for a short duration of a bindings call. The strings that use the memory wouldn't usually go anywhere and would be part of DOM, while you would deduct the memory used by them in the union destructor. Why don't we rather account them there? Additionally, with WTF::Strings there would potentially be an issue of double-accounting, as they're refcounted. So to me it looks like we're adding some notable overhead to a rather hot code path with no obvious benefits.
Francois Pierre DoraySomewhat agree.
If the string is unique and not double accounted then it could be nice to treat it as one object. (We have done so in other areas when useful)
I don't think it will solve the issue because even if it's large enough to make GC more likely, the GC will not get rid of the string (as the object is alive). Even if the GC reclaims other memory it's not synchronous and will likely not help for this call site.
Andrey Kosyakov"We're never retaining any of the unions that you've mentioned"
Unions are managed objects. Even if we're not retaining them, they're only freed through GC. Accounting for the external memory held by these unions ensures that GC decisions are based on accurate information."WTF::Strings there would potentially be an issue of double-accounting, as they're refcounted"
"If the string is unique and not double accounted then it could be nice to treat it as one object"
Yes, double-accounting could occur, if multiple unions acquire a reference to the same string, or if other objects with external memory accounting acquire a reference to the same string. A similar problem exists [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/message_event.cc;l=69;drc=e89a20d491d0946bdc64a17d5461c757b6954f4f) for example. We could solve it through [HasOneRef](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/text/string_impl.h;l=279-281;drc=e89a20d491d0946bdc64a17d5461c757b6954f4f) (i.e. only account for the memory if we're the only owner), or other solutions."I don't think it will solve the issue" Data points:
- This unaccounted memory is involved in 2.4% of all Chrome crashes (not just OOMs - go/ooms-from-text-content-or-outer-html).
- On average, 15% of the malloc'ed memory in renderers with >500MiB of malloc'ed memory is retained by these unions - go/heap-flame-graph-union-memory-feb2026.
- I confirmed that the current solution prevents growing memory usage to multiple GiB and OOM'ing on https://francoisdoray.github.io/web-repros/outer-html-leak.html.
Proposal: Land this and observe impact on OOMs and PMF. Based on observations, revert or refine (e.g. design a proper solution for the double-accounting).
Francois Pierre DorayAre unions other than those two mentioned a concern? Would it help if we make selected unions non-GC'ed? Since they're never retained, we could introduce an [ExtendedAttribute] to generate a non-GC'ed version of that minimizing heap thrashing somewhat, or, where possible, even replace their usage with overloads?
I’m concerned about the lifetime of strings returned from innerHTML (v8_element::InnerHTMLAttributeGetCallback), outerHTML (v8_element::InnerHTMLAttributeGetCallback), and textContent (v8_node::TextContentAttributeGetCallback). If there’s an alternative approach to ensure these strings aren't retained longer than necessary, I’m definitely open to suggestions!
Ah, sorry, I missed that we're talking about the return values! Ok, these three are an interesting bunch in particular -- as you can see, we only ever return strings in these unions, so, given these are hot calls, I'm tempted to implement some optimizations in bindings that bypass union creation altogether.
Now the question is, if we land your CL and it actually gives us improved memory usage, would such an optimization regress these gains? Perhaps we should account for strings returned to V8 on a different level, e.g. in [StringCahge](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/v8_value_cache.h;l=119)?
Related details
- Francois Pierre Doray
- Michael Lippautz
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 1 comment![]( "Open in Gerrit")
I'm not attached to this CL. I actually like your idea of bypassing union creation. Something like this? crrev.com/c/7618299
Whether we should have external memory accounting for strings owned by managed objects, and how to accomplish this, can be a separate discussion.
Related details
- Andrey Kosyakov
- Michael Lippautz