Using absl::flat_hash_map in headers and build size

[David Roger]

Hello,

my CL is failing the compile-size step, and I don't know how to fix this.

I think this is because it adds includes to basl::flat_hash_map in header files?

(for example in components/sync/service/data_type_manager.h or sync_service.h)

The instructions recommend using forward declarations, but I don't think this is desirable for flat_hash_map, right?

I used flat_hash_map because it's the newly recommended map type, but I see that it's not used widely yet.

Any suggestions?

Thank you!

[Greg Thompson]

I hit the same thing in https://crrev.com/c/6335513. "Compile-Size: See commit description." w/ an explanation in the description seems appropriate to me.

--
You received this message because you are subscribed to the Google Groups "build" group.
To unsubscribe from this group and stop receiving emails from it, send an email to build+un...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/build/CAL3rL5hekm%2B%2BXsy_7m%3D8J%2BTaJh-7pBQgnjtEuffHGc%2BbHWkkYQ%40mail.gmail.com.

[Junji Watanabe]

As Greg suggested, you can skip the check if the compile size increase is not avoidable.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/build/CAKQnr0SZNVnRLRiTcO2EQp4KoOcdAcS-Dwr7gUJaj3nvAAAMHQ%40mail.gmail.com.

[Takuto Ikuta (生田 拓人)]

I think some of your includes can be replaced with forward declaration?

e.g. https://chromium-review.googlesource.com/c/chromium/src/+/6429605/27/chrome/browser/ui/signin/signin_view_controller.h is the case of https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++-dos-and-donts.md#minimize-code-in-headers:~:text=Declaring%20a%20function%20that%20takes%20the%20type%20as%20a%20reference%20parameter%20(e.g.%20void%20f(MyClass%26%20arg)%3B). 

and include could be replaced with

```
namespace absl {
template<class K, class V> class flat_hash_map;
}
```

?

You need to include header in cc files instead if you use forward declaration in header files.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/build/CAPi%2BNeWs8L-s%3Dg9vnaDCX4QipfyJ1oH2Gscsghkp%3DEF9kjjN8Q%40mail.gmail.com.

--

Takuto Ikuta

Software Engineer in Tokyo

Chrome Ops (chrome browser build team)

[Hans Wennborg]

+1, You can forward declare flat_hash_map as Takuto showed.

I just skimmed the first couple of headers of your CL, and functions which return a flat_hash_map only need a forward declaration. I also saw some functions taking it by value as an argument. Perhaps those could be passed by const-ref instead.

The compile-time bot reported a 2.15 GB increase in input size, so it seems worth some effort if we can reduce that.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/build/CALNjmMq2oyJ6UCVFvD-WnACc5vmGN0t14zzFciWmSXCz_3qy7w%40mail.gmail.com.

[David Roger]

This does not work:

```
namespace absl {
template<class K, class V> class flat_hash_map;
}
```

The error is:

```

In file included from ../../components/sync/service/sync_service.h:23:
../../third_party/abseil-cpp/absl/container/flat_hash_map.h:130:1: error: too many template parameters in template redeclaration
  130 | template <class K, class V, class Hash = DefaultHashContainerHash<K>,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  131 |           class Eq = DefaultHashContainerEq<K>,
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  132 |           class Allocator = std::allocator<std::pair<const K, V>>>
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../components/sync/service/data_type_manager.h:22:1: note: previous template declaration is here
   22 | template <class K, class V>

```

This is because flat_hash_map has actually 6 template arguments, but 4 of them have default values.

If we wanted to forward declare, I think we would need a file like this one:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/vulkan-validation-layers/src/layers/external/parallel_hashmap/phmap_fwd_decl.h;l=85;drc=6b3cbe3f81357a47cdcac896bd9b145bcd3fa14e

[Hans Wennborg]

I suppose the Abseil folks aren't interested in providing a flat_hash_map_fwd.h header.

Oh well.