Binary Size implications of //base/containers recommendations

[Alex Cooper]

Hey Folks,

Not really sure the best place to start with this, but figured I'd start with you all as the most impacted.

I'm currently working on a CL https://chromium-review.googlesource.com/c/chromium/src/+/6860503 that triggered the binary size bot for being too large. One method in particular stood out to me as being a bit larger than expected. After investigation, I found that a simple switch of 3 items from absl::flat_hash_map, as recommended in https://source.chromium.org/chromium/chromium/src/+/main:base/containers/README.md, to std::map and one from absl::flat_hash_set to std::set, triggered a *3 KB* reduction, which feels like a lot for such simple usage.

I'm wondering if perhaps I was holding something wrong or if the guidance in the containers README should be updated for cases like whatever make mine unique?

Patchset 3 was what I had initially intended to be final, patchset 4 simplified things based on the default guidance and removed a couple of structs and maps and got me ~0.5kb savings, and then patchset 5 was due to my suspicions of seeing a lot of specialization code for the absl maps.

Thanks,

Alex Cooper

[Andrew Grieve]

SuperSize results of your patchsets, with "Group by: Template" checked: Using Map, Using FlatHashMap

One theory is that identical code folding does a better job with std::map, but I think it might just be that FlatHashMap is just bigger in terms of binary size :/

--
You received this message because you are subscribed to the Google Groups "binary-size" group.
To unsubscribe from this group and stop receiving emails from it, send an email to binary-size...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/binary-size/CAGOLbz3jcHvao_0mKAeJMu%2BOWQA-PxZawE3pYZ_YQqTfQuTG_g%40mail.gmail.com.

[Alex Cooper]

+Daniel Cheng 

I reached out to the #cxx folks and Daniel's theory was also that essentially for "first time" usages, there's a binary size tax to be paid as we weren't previously having to hash the items before, so presumably this is something that may get better over time.

[Daniel Cheng]

I think we should keep an eye on it, but I don't think it should be blocking.

I've previously done experiments to swap all unordered_map/unordered_set instances over to flat_hash_map/flat_hash_set, and the binary size impact was negligible in those cases.

That being said, the way absl prefers to define hash functions probably does lead to code being a bit more inlined... but that sort of thing doesn't tend to affect 32-bit Android builds as much, since the 32-bit Android build is very aggressive about not inlining things at all.

Daniel

[Alex Cooper]

FWIW, I didn't swap with `unordered_map` or set, but rather with the ordered versions, which while I didn't need the ordering property seemed the most likely options to reduce the size as the guidance was that the unordered versions were highly suboptimal.

[Daniel Cheng]

Sorry I should have been a bit clearer. I think the absl unordered containers and the STL unordered containers are comparable in terms of binary size. In Chrome, we wouldn't select between unordered standard containers and ordered standard containers based on binary size–we would select between them based on which one is the best fit. Thus, we shouldn't let binary size stop us from using absl unordered containers when they are the right choice.

Daniel

[Alex Cooper]

Sounds good, I will proceed with using `Binary-Size` to ignore the size increase on my change; I wanted to at least bring it up to this group since it was such a relatively large change for just changing the type and wanted to ensure that the folks who were concerned were aware. Please let me know if I should file some kind of tracking bug (and on which component/assigned to whom).

Thanks,

Alex