Add plugin option to skip complex constructor checks for aggregate types [chromium/src : main]
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Commit-Queue | +1 |
1 comment
Sorry for the wide audience. I finally got around to poking at this a bit after @davi...@chromium.org complained about this (yet again) 9 months ago or so. The original criteria (only public fields and no virtual methods) we had discussed more or less maps to "is a C++ aggregate", so the only extension is to make this aggregate logic apply to base classes as well. https://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html#a179a178d382b17321ff67e39b5641058 says base classes are not allowed, but as of C++23, aggregates may have base classes; however, a non-aggregate base class does not appear to disqualify a derived class from being an aggregate.
Originally I had also pushed for forcing destructors to still be out-of-line to reduce the binary size impact, but since this suppresses implicit move constructors, that seems likely to result in worse outcomes (see also crbug.com/365953516). So as proposed, this CL does not require out-of-line dtors for aggregate types.
As for binary size impact, I had gemini-cli trawl the codebase for candidates for converting to aggregates and then convert them. https://chromium-review.googlesource.com/c/chromium/src/+/7763599/ isn't perfect, but should be a rough proxy of the binary size impact.
Unsurprisingly, arm32 is not really affected; arm64 increases by almost 60KB. A local official build of Linux demonstrates about a 100KB increase. About 10KB of this was due to increases in RenderWidgetHostImpl::DragTargetDragEnter, while another 10KB or so is due to increases in NetworkContext::MakeURLRequestContext, since `HttpNetworkSessionParams` has a lot of fields.
Requiring out-of-line destructors halfs the binary size impact... but as mentioned previously, suppresses move constructors which is probably a net negative.
@ha...@chromium.org please do the overall review here; I'm also curious if you're worried about the potential effect on compile-time, since this does move some work to anyone that includes these headers.
@agr...@chromium.org do you have any concerns from a binary size perspective here?
And style guide owners, would be interested in hearing any feedback, concerns, or suggestions overall?
Related details
- Andrew Grieve
- David Benjamin
- Hans Wennborg
- Jeremy Roman
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve added 1 comment![]( "Open in Gerrit")
- Daniel Cheng
- David Benjamin
- Hans Wennborg
- Jeremy Roman
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Kasting (Gerrit)
Peter Kasting added 1 comment![]( "Open in Gerrit")
༼ つ ◕_◕ ༽つ Take my energy ༼ つ ◕_◕ ༽つ
Tom Sepez (Gerrit)
Tom Sepez voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Daniel Cheng
- David Benjamin
- Hans Wennborg
- Jeremy Roman
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
Super, but also get approval from Nico, say, before landing, just to keep us all on the same page.
David Benjamin (Gerrit)
David Benjamin voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Benjamin
| Code-Review | +1 |
Sorry for the wide audience. I finally got around to poking at this a bit after @davi...@chromium.org complained about this (yet again) 9 months ago or so. The original criteria (only public fields and no virtual methods) we had discussed more or less maps to "is a C++ aggregate", so the only extension is to make this aggregate logic apply to base classes as well. https://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html#a179a178d382b17321ff67e39b5641058 says base classes are not allowed, but as of C++23, aggregates may have base classes; however, a non-aggregate base class does not appear to disqualify a derived class from being an aggregate.
Originally I had also pushed for forcing destructors to still be out-of-line to reduce the binary size impact, but since this suppresses implicit move constructors, that seems likely to result in worse outcomes (see also crbug.com/365953516). So as proposed, this CL does not require out-of-line dtors for aggregate types.
As for binary size impact, I had gemini-cli trawl the codebase for candidates for converting to aggregates and then convert them. https://chromium-review.googlesource.com/c/chromium/src/+/7763599/ isn't perfect, but should be a rough proxy of the binary size impact.
Unsurprisingly, arm32 is not really affected; arm64 increases by almost 60KB. A local official build of Linux demonstrates about a 100KB increase. About 10KB of this was due to increases in RenderWidgetHostImpl::DragTargetDragEnter, while another 10KB or so is due to increases in NetworkContext::MakeURLRequestContext, since `HttpNetworkSessionParams` has a lot of fields.
Requiring out-of-line destructors halfs the binary size impact... but as mentioned previously, suppresses move constructors which is probably a net negative.
@ha...@chromium.org please do the overall review here; I'm also curious if you're worried about the potential effect on compile-time, since this does move some work to anyone that includes these headers.
@agr...@chromium.org do you have any concerns from a binary size perspective here?And style guide owners, would be interested in hearing any feedback, concerns, or suggestions overall?
I'm not familiar enough with Clang to do a great review of this CL code-wise, but I glanced over it and it seems plausible? As far as C++ practices goes, I wholeheartedly support this change. 😊
Related details
- Daniel Cheng
Hans Wennborg (Gerrit)
Hans Wennborg added 3 comments![]( "Open in Gerrit")
This seems good to me if it doesn't regress build time/size too much. Do you have any numbers on this?
prototype and measure the potential future impact on Chrome binary size.What were the results of the measurements?
// No constructors should be triggered since `DerivedAggregate` is still ans/No constructors/No warning/ ?
Related details
- Daniel Cheng
- Jeremy Roman
- Nico Weber
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
This seems good to me if it doesn't regress build time/size too much. Do you have any numbers on this?
Yes, see the initial message in https://chromium-review.googlesource.com/c/chromium/src/+/7763597/comments/929e267e_9afa0cc5. I'll mention them in the CL description on my next upload too.
Related details
- Hans Wennborg
- Jeremy Roman
- Nico Weber
Jeremy Roman (Gerrit)
Jeremy Roman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jeremy Roman
| Code-Review | +1 |
1 comment
Seems fine to me. I'm a bit worried that there are some large structs where this would be an issue, but while this carveout is a bit arbitrary it's not unreasonable.
It would be nice if there were a way we could periodically understand which aggregates, if any, are adding a lot of weight to the build, but that doesn't seem blocking.
Related details
- Daniel Cheng
- Hans Wennborg
- Nico Weber
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
Seems fine to me. I'm a bit worried that there are some large structs where this would be an issue, but while this carveout is a bit arbitrary it's not unreasonable.
It would be nice if there were a way we could periodically understand which aggregates, if any, are adding a lot of weight to the build, but that doesn't seem blocking.
If we find some class of aggregates are impacting binary size, we can also try to improve the compiler's inlining heuristic. IMO it's a bug when giving the compiler more information causes it to generate less code. (Of course, engineering realities mean it's sometimes right for us to work around those bugs instead of fixing the source. But, at least in principle, we *shouldn't* need this for binary size.)
(Build times are less clear. IMO the principled answer is we should invest some % of our time worrying about build times into getting C++20 modules to work, but that is a dream even more burdened with engineering realities. 😭)
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
prototype and measure the potential future impact on Chrome binary size.What were the results of the measurements?
Added a summary of some of the top regressions to the CL description now. The impact is definitely unevenly distributed; `DropDataToMetaData()` (inlined into `RenderWidgetHostImpl::DragTargetDragEnter()`) gets hit disproportionately hard.
// No constructors should be triggered since `DerivedAggregate` is still anDaniel Chengs/No constructors/No warning/ ?
Done
Related details
- Hans Wennborg
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hans Wennborg (Gerrit)
Hans Wennborg voted Code-Review+1![]( "Open in Gerrit")
- Daniel Cheng
- Nico Weber
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
I talked to thakis@ offline, who was OK with this if it didn't regress build performance. So I did some build measurements; apparently one of the motivations for the original check was build times on Windows, especially in component builds. thakis@ also pointed at https://blog.llvm.org/2018/11/30-faster-windows-builds-with-clang-cl_14.html which should have helped mitigate the original build time issues.
Using a only-local dcheck-always-on component release cross-compile for Windows on Linux, with and without https://chromium-review.googlesource.com/c/chromium/src/+/7763599, which converts 61 types to aggregates. I disabled the plugin checks for simplicity, and the cycle counts before and after are fairly comparable and within the likely effect of noise. I repeated the test in two directions as well (building before first, and hten after, and then after and then first to try to eliminate caching/ordering effects).
Related details
- Nico Weber
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Commit-Queue | +2 |
David BenjaminSeems fine to me. I'm a bit worried that there are some large structs where this would be an issue, but while this carveout is a bit arbitrary it's not unreasonable.
It would be nice if there were a way we could periodically understand which aggregates, if any, are adding a lot of weight to the build, but that doesn't seem blocking.
If we find some class of aggregates are impacting binary size, we can also try to improve the compiler's inlining heuristic. IMO it's a bug when giving the compiler more information causes it to generate less code. (Of course, engineering realities mean it's sometimes right for us to work around those bugs instead of fixing the source. But, at least in principle, we *shouldn't* need this for binary size.)
(Build times are less clear. IMO the principled answer is we should invest some % of our time worrying about build times into getting C++20 modules to work, but that is a dream even more burdened with engineering realities. 😭)
There are definitely some structs where the effect is quite immediately noticeable. I'm going to experiment with some ways to try to find these sorts of more expensive structs. But whatever we can do seems like it will be a workaround to trying to improve the compiler's behavior in this area, and/or encouraging patterns that generate less bloat.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add plugin option to skip complex constructor checks for aggregate types
With this option enabled, the Chrome clang plugin completely skips the
complex constructor and destructor checks for aggregate types. This
allows the use of designated initializers, which can be a very
convenient shorthand.
While there is some binary size and compile time cost, this carveout is
limited to classes and structs that meet the C++23 requirements for an
aggregate. Most importantly, this means no virtual methods and only
public fields.
In addition, the C++ style guide also specifically recommends structs
only when a type does not need to maintain any internal invariants:
between this guidance and the previous restrictions, this should provide
an acceptable balance between utility and cost.
As an aside, the original plan was to require out-of-line destructors
for aggregates; however, this prevents the compiler from providing an
implicitly-declared move constructor where appropriate and leads to
worse overall outcomes.
Some numbers from converting 61 candidate types to use designated
initializers:
- arm32 binary size isn't affected
- arm64 binary size increases by ~60KB
- official Linux builds increase by ~100KB. This increase is not evenly
distributed, e.g.:
- `RenderWidgetHostImpl::DragTargetDragEnter()` increases in size from
879 bytes to 10,824 bytes. This is the largest increase; before and
after, the compiler inlines the body of the `DropDataToMetaData()`
helper function, which includes multiple calls to `~Metadata()` to
destroy the temporaries; however, in the after version, clang emits
the code for the destructor inline, causing significant bloat.
Rewriting this to use `emplace_back()` to avoid the temporaries
helps reduce the impact to only 3,938 bytes, but does not eliminate
it entirely.
- `NetworkContext::MakeURLRequestContext()` was previously inlined
into `NetworkContext`; the combined size before is 15,313 bytes,
while the combined size after is 16,978 bytes. This changer is
primarily due to `HttpNetworkSessionParams` becoming an aggregate;
the destructor is large due to various fields that are maps.
gemini-cli used to develop initial plugin changes and tests, as well as
prototype and measure the potential future impact on Chrome binary size.
- M tools/clang/plugins/FindBadConstructsAction.cpp
- M tools/clang/plugins/FindBadConstructsConsumer.cpp
- M tools/clang/plugins/FindBadConstructsConsumer.h
- M tools/clang/plugins/Options.h
- A tools/clang/plugins/tests/aggregate_ctor.cpp
- A tools/clang/plugins/tests/aggregate_ctor.flags
- A tools/clang/plugins/tests/aggregate_ctor.h
- A tools/clang/plugins/tests/aggregate_ctor.txt
- A tools/clang/plugins/tests/inline_copy_ctor.flags
- A tools/clang/plugins/tests/inline_ctor.flags
- M tools/clang/plugins/tests/inline_ctor.txt
- A tools/clang/plugins/tests/missing_ctor.flags
- M tools/clang/plugins/tests/missing_ctor_dllexport.flags
- A tools/clang/plugins/tests/missing_ctor_ignored_base.flags
- A tools/clang/plugins/tests/nested_class_inline_ctor.flags
Code-Review: +1 by David Benjamin, +1 by Tom Sepez, +1 by Jeremy Roman, +1 by Hans Wennborg
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |