1437872 - Vector::growStorageBy produces bloated code

Status: NEW

(Core :: MFBT, enhancement, P3)

Description

[Nathan Froyd [:froydnj]]

Adding up growStorageBy instantiations on a local optimized Linux build:

froydnj@hawkeye:/opt/build/froydnj/build-icecc-mc$ readelf -sW dist/bin/libxul.so |grep growStorageBy |f 3 |addup
301044

So ~300K on local builds, which turns into ~190K on nightlies (see #2 below). The numbers are similar on Mac (see bug 1437063 comment 5), except nightlies don't get any better.  There are some space gains to be had here.

Bug 1437063 comment 7 has a more extensive write-up, but the basic ideas are:

1) We have separate growStorageBy instantiations for differing inline storage amounts, even with the same element type and allocator.

2) We have separate growStorageBy instantiations for different pointer types.  On Linux/Windows/Android, the linker (presumably) cleans that up for us, but we don't have the same optimization on Mac.

3) The JS engine particularly has several different allocators that require separate instantiations of growStorageBy.

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1437872 - Reduce bloat of mozilla::Vector by reducing dependency on template parameters, part 1. r=froydnj

Comment 2

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1437872 - Reduce bloat of mozilla::Vector by reducing dependency on template parameters, part 2. r=froydnj

Depends on D83518

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1437872 - Do not MOZ_NEVER_INLINE growStorageBy. r=froydnj

Depends on D83519

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1437872 - Use void* storage for all pointer types in mozilla::Vector. r=froydnj

Depends on D83520

Comment 5

[Simon Giesecke [:sg] [he/him]]

With the first three patches attached, I get a small text/rodata reduction, a small VM size increase, but a rather large debug info reduction, with a local optimized build, but without LTO (which is not working for me currently):

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0%  +113Ki  [ = ]       0    .debug_str
  +0.0% +15.0Ki  [ = ]       0    .strtab
  +0.0% +10.1Ki  +0.0% +10.1Ki    .eh_frame
  +0.0% +4.17Ki  [ = ]       0    .symtab
  +0.0% +1.41Ki  +0.0% +1.41Ki    .eh_frame_hdr
  -0.0% -1.20Ki  [ = ]       0    .debug_abbrev
 -13.6% -1.37Ki  [ = ]       0    [Unmapped]
  -0.0% -2.77Ki  -0.0% -2.77Ki    .text
  -0.0% -3.38Ki  -0.0% -3.38Ki    .rodata
  -0.2%  -181Ki  [ = ]       0    .debug_line
  -0.2% -1.79Mi  [ = ]       0    .debug_info
  -4.9% -2.95Mi  [ = ]       0    .debug_ranges
  -1.0% -4.19Mi  [ = ]       0    .debug_loc
  -0.4% -8.97Mi  +0.0% +5.38Ki    TOTAL

When including also the fourth patch (factoring out storage and using a common VectorStorage instance for pointer types), I get an actual VM size reduction:

[simon@sigibln fuzzy]$ ~/src/bloaty/build/bloaty libxul-vector-bloat-removed-pointer-storage.so -- libxul-vector-bloat-removal-baseline.so 
    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.1%  +123Ki  [ = ]       0    .debug_pubtypes
  +0.0% +13.1Ki  [ = ]       0    .strtab
  +0.0% +6.65Ki  +0.0% +6.65Ki    .eh_frame
  +0.0% +2.39Ki  [ = ]       0    .symtab
   +39% +2.01Ki  [ = ]       0    [Unmapped]
  +0.0%    +808  +0.0%    +808    .eh_frame_hdr
  +0.0%    +750  [ = ]       0    .debug_pubnames
  +0.0%    +157  +0.0%    +157    .dynstr
  +0.0%    +136  [ = ]       0    .debug_frame
  +0.0%     +52  +0.0%     +34    [4 Others]
  +0.0%     +48  [ = ]       0    .debug_aranges
  +0.0%     +48  +0.0%     +48    .rela.dyn
  +0.0%     +32  +0.0%     +32    .data.rel.ro
  -5.3%    -239  -5.3%    -239    [LOAD #1 [R]]
  -0.0% -3.31Ki  -0.0% -3.31Ki    .rodata
  -0.0% -50.2Ki  -0.0% -50.2Ki    .text
  -0.1%  -166Ki  [ = ]       0    .debug_str
  -0.2%  -189Ki  [ = ]       0    .debug_line
  -0.3% -2.36Mi  [ = ]       0    .debug_info
  -4.9% -2.99Mi  [ = ]       0    .debug_ranges
  -1.0% -4.26Mi  [ = ]       0    .debug_loc
  -0.5% -9.85Mi  -0.0% -46.0Ki    TOTAL

Nathan, what do you think about that? I can try to rebase the VectorStorage patch to apply independently of the others, and see what the effect with that is.

Comment 6

[Simon Giesecke [:sg] [he/him]]

With a LTO build and all patches applied, the improvement is even a bit larger:

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0%    +157  +0.0%    +157    .dynstr
  +0.0%    +153  [ = ]       0    .debug_pubtypes
  +0.0%    +136  [ = ]       0    .debug_frame
  +0.0%     +56  +0.0%     +30    [4 Others]
  +0.0%     +48  [ = ]       0    .debug_aranges
  +0.0%     +48  +0.0%     +48    .rela.dyn
  +0.0%     +32  +0.0%     +32    .data.rel.ro
 -20.2%    -235 -20.2%    -235    [LOAD #1 [R]]
  -0.0% -1.12Ki  -0.0% -1.12Ki    .eh_frame_hdr
  -0.0% -1.20Ki  [ = ]       0    .debug_abbrev
  -0.0% -3.16Ki  -0.0% -3.16Ki    .rodata
  -0.0% -3.66Ki  [ = ]       0    .symtab
 -66.2% -4.06Ki  [ = ]       0    [Unmapped]
  -0.0% -5.32Ki  -0.0% -5.32Ki    .eh_frame
  -0.0% -12.5Ki  [ = ]       0    .strtab
  -0.2%  -162Ki  -0.2%  -162Ki    .text
  -0.1%  -174Ki  [ = ]       0    .debug_str
  -0.2%  -197Ki  [ = ]       0    .debug_line
  -0.3% -2.13Mi  [ = ]       0    .debug_info
  -4.3% -2.44Mi  [ = ]       0    .debug_ranges
  -1.0% -4.02Mi  [ = ]       0    .debug_loc
  -0.4% -9.13Mi  -0.1%  -171Ki    TOTAL

On the symbol level, this breaks down to:

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  [NEW]  +134Ki  [NEW]  +115Ki    mozilla::detail::VectorStorage<>::growStorageBy()
  +184% +35.2Ki  +185% +35.2Ki    js::jit::CacheIRCloner::cloneOp()
  [NEW] +15.1Ki  [NEW] +15.0Ki    js::gc::GCRuntime::beginMarkPhase()
  [NEW] +8.98Ki  [NEW] +7.61Ki    mozilla::detail::VectorStorage<>::growTo()
  +169% +8.94Ki  +172% +8.94Ki    js::ctypes::CType::GetName()
  [NEW] +8.05Ki  [NEW] +6.76Ki    mozilla::detail::VectorImplCommon<>::convertToHeapStorage()
  +120% +7.54Ki  +121% +7.54Ki    ShortestPaths()
  [DEL] -5.97Ki  [DEL] -5.19Ki    mozilla::Vector<>::convertToHeapStorage()
  [DEL] -7.42Ki  [DEL] -6.38Ki    mozilla::detail::VectorImpl<>::growTo()
 -55.5% -8.98Ki -58.2% -8.98Ki    AtomicEffectOp<>()
  -0.0% -5.47Ki  -0.1% -11.0Ki    [3655 Others]
 -16.1% -11.8Ki -16.1% -11.8Ki    js::jit::BaselineInterpreterGenerator::emitInterpreterLoop()
 -46.4% -12.7Ki -47.8% -12.7Ki    AtomicFetchOp<>()
 -47.0% -13.5Ki -47.2% -13.5Ki    js::gc::GCRuntime::incrementalSlice()
 -10.6% -20.5Ki -10.6% -20.5Ki    js::jit::CodeGenerator::generateBody()
  -0.1%  -174Ki  [ = ]       0    [section .debug_str]
  -0.2%  -197Ki  [ = ]       0    [section .debug_line]
  [DEL]  -324Ki  [DEL]  -278Ki    mozilla::Vector<>::growStorageBy()
  -0.3% -2.13Mi  [ = ]       0    [section .debug_info]
  -4.3% -2.44Mi  [ = ]       0    [section .debug_ranges]
  -1.0% -4.02Mi  [ = ]       0    [section .debug_loc]
  -0.4% -9.13Mi  -0.1%  -171Ki    TOTAL

Interestingly, the size of some functions (js::jit::BaselineInterpreterGenerator::emitInterpreterLoop, js::gc::GCRuntime::incrementalSlice, js::jit::CodeGenerator::generateBody) decreases significantly, which seems that removing MOZ_NEVER_INLINE enables some optimizations that reduce the size?

Comment 7

[Nathan Froyd [:froydnj]]

I think these patches are generally worth pursuing.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.