Issue metadata
Sign in to add a comment
|
103 KB (0.1%) regression in resource_sizes (MonochromePublic.apk) at 555449:555449 |
||||||||||||||||||||||
Issue descriptionCaused by: [oilpan] Enable incremental marking buildflag (but then reverted) Original commit: d8a31d453aaa429829d6ba183f94b5ba016768bd Revert commit: b1940e0ce70651ca8cb36787ed0a96ace34eace3 It's not clear whether or not this increase was expected, but, the revert cites suspected breakage of unit tests (not binary size impact). The size impact is also substantial. Before re-landing, could you indicate on this bug whether such an increase is expected or not?
,
May 3 2018
Assigning to mlippautz@chromium.org because this is the only CL in range: [oilpan] Enable incremental marking buildflag Includes incremental garbage collection infrastructure in regular builds. Does not enable incremental marking at runtime! This CL may cause throughput regressions and is an attempt to collect a broad range of potential performance issues. Bug: chromium:757440 Change-Id: Ie7b75f8d96f9230400beed1a3258e45e0ca742ed Reviewed-on: https://chromium-review.googlesource.com/980753 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#555449}
,
May 3 2018
Looks like it re-landed at commit bd9ac13acc9d99a4f192b2734613545c187a319c.
,
May 3 2018
,
May 4 2018
The increase is definitely expected, sorry for not adding that to the CL description. The infrastructure added will allow us to do incremental and ultimately concurrent garbage collection. Long-term, we will be able to get rid of some memory which is currently implemented as duplicated code. That's part of the unified heap project.
,
May 4 2018
Sounds good. Based on that, closing this off.
,
May 7 2018
,
May 10 2018
Sorry to be a pain here, but 100kb is a fairly large jump (worth spending a few days investigating), and it's not clear what has been done to try an minimize the size. Totally agree that the feature is important, but it'd be good to show that some effort has been made to minimize the code size for it (E.g. look at the symbol diff or a bit of disassembly). Here's some output from tools/binary_size/diagnose_bloat.py --cloud d8a31d453aaa429829d6ba183f94b5ba016768bd (ignore first 3 symbols): Section Legend: t=.text, r=.rodata, R=.data.rel.ro, d=.data, b=.bss, x=.dex, m=.dex.method, p=.pak.translations, P=.pak.nontranslated, o=.other Index | Running Total | Section@Address | Δ PSS (Δ size_without_padding) | Path ------------------------------------------------------------ ~ 0) 9292 (8.3%) o@0x0 9292 (1968150->1977442) base/trace_event/cfi_backtrace_android.cc unwind_cfi_32 ~ 1) 7852 (7.0%) o@0x0 -1440 (0->0) {{no path}} Overhead: ELF file ~ 2) 6752 (6.0%) o@0x0 -1100 (0->0) {{no path}} Overhead: APK file ~ 3) 7656 (6.9%) t@Group 904 (77184->78088) {{no path}} ** thunk (count=2) ~ 4) 7996 (7.2%) t@0x1e789b4 340 (140->480) third_party/blink/renderer/core/dom/container_node.cc blink::ContainerNode::RemoveBetween ~ 5) 8238 (7.4%) t@0x1f01110 242 (896->1138) third_party/blink/renderer/core/editing/suggestion/text_suggestion_controller.cc std::__ndk1::__sort<> ~ 6) 8466 (7.6%) t@0x2304210 228 (792->1020) third_party/blink/renderer/modules/media_controls/media_controls_impl.cc blink::MediaControlsImpl::InitializeControls ~ 7) 8692 (7.8%) t@0x1ddee54 226 (812->1038) third_party/blink/renderer/core/css/active_style_sheets.cc std::__ndk1::__sort<> ~ 8) 8910 (8.0%) t@0x1ddf262 218 (190->408) third_party/blink/renderer/core/css/active_style_sheets.cc std::__ndk1::__sort3<> ~ 9) 9122 (8.2%) t@0x1e4cc52 212 (11->224) third_party/blink/renderer/core/css/property_registry.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 10) 9335 (8.4%) t@0x1e63152 212 (11->224) third_party/blink/renderer/core/css/rule_set.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 11) 9548 (8.6%) t@0x1e63f96 212 (11->224) third_party/blink/renderer/core/css/rule_set.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 12) 9761 (8.7%) t@0x1e6cec0 212 (11->224) third_party/blink/renderer/core/css/style_engine.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 13) 9974 (8.9%) t@0x1eba762 212 (11->224) third_party/blink/renderer/core/dom/tree_ordered_map.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 14) 10187 (9.1%) t@0x1f7bd40 212 (11->224) third_party/blink/renderer/core/html/custom/custom_element_registry.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 15) 10400 (9.3%) t@0x1f7c4ac 212 (11->224) third_party/blink/renderer/core/html/custom/custom_element_registry.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 16) 10613 (9.5%) t@0x1fbd138 212 (11->224) third_party/blink/renderer/core/html/forms/html_form_element.cc ~ 15) 10400 (9.3%) t@0x1f7c4ac 212 (11->224) third_party/blink/renderer/core/html/custom/custom_element_registry.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 16) 10613 (9.5%) t@0x1fbd138 212 (11->224) third_party/blink/renderer/core/html/forms/html_form_element.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 17) 10826 (9.7%) t@0x1fcc8b4 212 (11->224) third_party/blink/renderer/core/html/forms/radio_button_group_scope.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 18) 11039 (9.9%) t@0x2055436 212 (11->224) third_party/blink/renderer/core/layout/custom/pending_layout_registry.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 19) 11252 (10.1%) t@0x2182464 212 (11->224) third_party/blink/renderer/core/svg/graphics/filters/svg_filter_builder.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 20) 11465 (10.3%) t@0x21a4dfc 212 (11->224) third_party/blink/renderer/core/svg/svg_tree_scope_resources.cc WTF::HashTable<>::RehashTo (num_aliases=19->1) ~ 21) 11673 (10.5%) t@0x1fff6b0 208 (1044->1252) third_party/blink/renderer/core/html/track/cue_timeline.cc std::__ndk1::__sort<> ~ 22) 11869 (10.6%) t@0x1f01582 196 (190->386) third_party/blink/renderer/core/editing/suggestion/text_suggestion_controller.cc std::__ndk1::__sort3<> ~ 23) 12063 (10.8%) t@0x1f89860 194 (246->440) third_party/blink/renderer/core/html/parser/html_construction_site.cc blink::HTMLConstructionSite::AttachLater ~ 24) 12249 (11.0%) t@0x1ddf514 186 (196->382) third_party/blink/renderer/core/css/active_style_sheets.cc std::__ndk1::__sort5<> ~ 25) 12077 (10.8%) t@0x1f8bf50 -172 (684->512) third_party/blink/renderer/core/html/parser/html_construction_site.cc WTF::VectorBuffer<>::SwapVectorBuffer ~ 26) 12241 (11.0%) t@0x1f89a54 164 (616->780) third_party/blink/renderer/core/html/parser/html_construction_site.cc Things to look at: * Why does RehashTo no longer get identical-code-folded? * Why did sort symbols get bigger? Here are some of the symbols with parameters (you can see why we strip the by default :P) (via supersize console's Print(Diff(), verbose=True)): ~ 5) 8238 (7.4%) t@0x1f01110 242 (896->1138) num_aliases=1 source_path=third_party/blink/renderer/core/editing/suggestion/text_suggestion_controller.cc object_path=third_party/blink/renderer/core/editing/editing_4/text_suggestion_controller.o flags={} name=std::__ndk1::__sort<> full_name=std::__ndk1::__sort<blink::ComputeSuggestionInfos(blink::HeapVector<std::__ndk1::pair<blink::Member<blink::Node>, blink::Member<blink::DocumentMarker> >, 0u> const&, unsigned int)::$_0&, std::__ndk1::pair<blink::Member<blink::Node>, blink::Member<blink::DocumentMarker> >*>(std::__ndk1::pair<blink::Member<blink::Node>, blink::Member<blink::DocumentMarker> >*, std::__ndk1::pair<blink::Member<blink::Node>, blink::Member<blink::DocumentMarker> >*, blink::ComputeSuggestionInfos(blink::HeapVector<std::__ndk1::pair<blink::Member<blink::Node>, blink::Member<blink::DocumentMarker> >, 0u> const&, unsigned int)::$_0&) ~ 6) 8466 (7.6%) t@0x2304210 228 (792->1020) num_aliases=1 source_path=third_party/blink/renderer/modules/media_controls/media_controls_impl.cc object_path=third_party/blink/renderer/modules/media_controls/media_controls/media_controls_impl.o flags={} name=blink::MediaControlsImpl::InitializeControls full_name=blink::MediaControlsImpl::InitializeControls() ~ 7) 8692 (7.8%) t@0x1ddee54 226 (812->1038) num_aliases=1 source_path=third_party/blink/renderer/core/css/active_style_sheets.cc object_path=third_party/blink/renderer/core/css/css_0/active_style_sheets.o flags={} name=std::__ndk1::__sort<> full_name=std::__ndk1::__sort<std::__ndk1::__less<std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >, std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> > >&, std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >*>(std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >*, std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >*, std::__ndk1::__less<std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >, std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> > >&) ~ 8) 8910 (8.0%) t@0x1ddf262 218 (190->408) num_aliases=1 source_path=third_party/blink/renderer/core/css/active_style_sheets.cc object_path=third_party/blink/renderer/core/css/css_0/active_style_sheets.o flags={} name=std::__ndk1::__sort3<> full_name=std::__ndk1::__sort3<std::__ndk1::__less<std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >, std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> > >&, std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >*>(std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >*, std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >*, std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >*, std::__ndk1::__less<std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> >, std::__ndk1::pair<blink::Member<blink::CSSStyleSheet>, blink::Member<blink::RuleSet> > >&) ~ 9) 9122 (8.2%) t@0x1e4cc52 212 (11->224) num_aliases=19->1 source_path=third_party/blink/renderer/core/css/property_registry.cc object_path=third_party/blink/renderer/core/css/css_8/property_registry.o flags={} name=WTF::HashTable<>::RehashTo full_name=WTF::HashTable<WTF::AtomicString, WTF::KeyValuePair<WTF::AtomicString, blink::Member<blink::PropertyRegistration> >, WTF::KeyValuePairKeyExtractor, WTF::AtomicStringHash, WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicString>, WTF::HashTraits<blink::Member<blink::PropertyRegistration> > >, WTF::HashTraits<WTF::AtomicString>, blink::HeapAllocator>::RehashTo(WTF::KeyValuePair<WTF::AtomicString, blink::Member<blink::PropertyRegistration> >*, unsigned int, WTF::KeyValuePair<WTF::AtomicString, blink::Member<blink::PropertyRegistration> >*) ~ 10) 9335 (8.4%) t@0x1e63152 212 (11->224) num_aliases=19->1 source_path=third_party/blink/renderer/core/css/rule_set.cc object_path=third_party/blink/renderer/core/css/css_9/rule_set.o flags={} name=WTF::HashTable<>::RehashTo full_name=WTF::HashTable<WTF::AtomicString, WTF::KeyValuePair<WTF::AtomicString, blink::Member<blink::HeapLinkedStack<blink::RuleData> > >, WTF::KeyValuePairKeyExtractor, WTF::AtomicStringHash, WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicString>, WTF::HashTraits<blink::Member<blink::HeapLinkedStack<blink::RuleData> > > >, WTF::HashTraits<WTF::AtomicString>, blink::HeapAllocator>::RehashTo(WTF::KeyValuePair<WTF::AtomicString, blink::Member<blink::HeapLinkedStack<blink::RuleData> > >*, unsigned int, WTF::KeyValuePair<WTF::AtomicString, blink::Member<blink::HeapLinkedStack<blink::RuleData> > >*) ~ 11) 9548 (8.6%) t@0x1e63f96 212 (11->224) num_aliases=19->1 source_path=third_party/blink/renderer/core/css/rule_set.cc object_path=third_party/blink/renderer/core/css/css_9/rule_set.o flags={} name=WTF::HashTable<>::RehashTo full_name=WTF::HashTable<WTF::AtomicString, WTF::KeyValuePair<WTF::AtomicString, blink::Member<blink::HeapTerminatedArray<blink::RuleData> > >, WTF::KeyValuePairKeyExtractor, WTF::AtomicStringHash, WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicString>, WTF::HashTraits<blink::Member<blink::HeapTerminatedArray<blink::RuleData> > > >, WTF::HashTraits<WTF::AtomicString>, blink::HeapAllocator>::RehashTo(WTF::KeyValuePair<WTF::AtomicString, blink::Member<blink::HeapTerminatedArray<blink::RuleData> > >*, unsigned int, WTF::KeyValuePair<WTF::AtomicString, blink::Member<blink::HeapTerminatedArray<blink::RuleData> > >*)
,
May 11 2018
Mid-term this effort will get rid of wrapper tracing which should decrease APK size. Michael, can you have a quick look if we regressed APK size somewhere for no good reason? I am definitely expecting a regression on that metric and would vote for won't fix if there is nothing obviously broken.
,
May 11 2018
tl;dr: Working as expected but I will double-check if we can optimize the RehashTo case. The change adds write barriers, i.e., writes to Oilpan-managed objects through the managed pointers are intercepted. Binary size increase on anything that does a write, e.g., std::sort or RehashTo, is expected when inlining is aggressive. We want aggressive inlining for best performance here. We distinguish the following cases: 1. Regular writes to managed pointers such as Member<T>: They should be trivially alias-able. 2. Write to something such as std::pair<Member<A>, Member<B>>: These require immediately executing some custom Trace() that forwards to both Member<A> and Member<B>. These can be aliased but only if they are inlined far enough. 3. Writes to something even more complicated such as std::pair<AtomicString, Member<A>>. This one cannot be aliased to anything useful as they signature is likely unique. Previously aliasing might have been possible as long as constructors/destructors were not inlined or completely trivial. Both, std::sort and RehashTo, show custom types (e.g. [1], [2]) on all the examples mentioned. With good inlinling, there's nothing we can do here to prevent binary size increase. Having said that, I will take a look if all those barriers are still required. It looks to me like at least in RehashTo we could do something differently and regain aliasing and decrease its size. [1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/editing/suggestion/text_suggestion_controller.cc?q=ComputeSuggestionInfos&l=118 [2] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/active_style_sheets.h?dr&l=17
,
May 14 2018
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/984762d2100f6aff3dacb8507595b813e68e96a5 commit 984762d2100f6aff3dacb8507595b813e68e96a5 Author: Michael Lippautz <mlippautz@chromium.org> Date: Tue May 15 15:33:20 2018 [oilpan,wtf] Rework HashTable write barriers Instead of emitting write barriers for single writes in HashTable, conservatively emit a barrier for the backing store during rehashing. This moves us in the tradeoff space: - Removes 40KiB of binary bloat - Avoids emitting barriers when re-inserting single elements which improves throughput for the case where incremental marking is off - Emits a single conservative barrier at the end of RehashTo. This potentially means that we (re-)scan more objects when incremental marking is on Bug: chromium:839514 , chromium:757440 Change-Id: Ib5495c9e2210836424d7b26f2138d852f8dafec7 Reviewed-on: https://chromium-review.googlesource.com/1055400 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#558713} [modify] https://crrev.com/984762d2100f6aff3dacb8507595b813e68e96a5/third_party/blink/renderer/platform/heap/heap_allocator.h [modify] https://crrev.com/984762d2100f6aff3dacb8507595b813e68e96a5/third_party/blink/renderer/platform/heap/marking_visitor.h [modify] https://crrev.com/984762d2100f6aff3dacb8507595b813e68e96a5/third_party/blink/renderer/platform/wtf/allocator/partition_allocator.h [modify] https://crrev.com/984762d2100f6aff3dacb8507595b813e68e96a5/third_party/blink/renderer/platform/wtf/hash_table.h
,
May 16 2018
#12 gave us back ~37Kib on the bots. https://chromeperf.appspot.com/group_report?rev=558713
,
May 16 2018
Nice work! Going to close as "we've put a non-trivial amount of effort into it", but feel free to reopen if you think there's more that can be done. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, May 3 2018