Use private virtual key_function() to address -Wweak-vtables

[eramongodb]

Followup to #1469 per #1469 (comment).

Note

This PR explores an alternative design proposal. Rejecting this proposal is an acceptable outcome.

For polymorphic classes with no out-of-line virtual function available to act as the key function for the class' vtable, this PR proposes using a dedicated private and not-exported virtual function named key_function() instead.

  typeinfo for bsoncxx::v1::exception
  vtable for bsoncxx::v1::exception
  typeinfo name for bsoncxx::v1::exception
- bsoncxx::v1::exception::~exception()

  typeinfo for mongocxx::v1::exception
  vtable for mongocxx::v1::exception
  typeinfo name for mongocxx::v1::exception
- mongocxx::v1::exception::~exception()

This pattern avoids Ro5 boilerplate and avoids exporting virtual destructors in the stable ABI. However, this pattern comes at the cost of increasing the size of the vtable with an otherwise-useless function and locking-in the key_function() into the stable ABI: even if the function itself is not exported, the function is part of the vtable, and the vtable is part of the stable ABI, so key_function() is part of the stable ABI by transitivity (removal == ABI breaking change).

Which of the following options do we prefer?

• Avoid Ro5 boilerplate and out-of-line defaulted virtual destructors. (Accept this PR.)
• Avoid ABI-stability-supporting a "useless" function in the vtable. (Reject this PR.)

[vector-of-bool]

Alternatively: Do we actually care about the weak-vtables warning? AFAIK, all modern linkers do comdat merging up-front, and a single instance of a single class across a few files feels unlikely to have a large build performance impact.

[eramongodb]

Do we actually care about the -Wweak-vtables warning?

I do not have any emperical numbers concerning its effect on our codebase.

However, per the linked Itanium ABI spec (our dtors are not pure virtual, but this recommendation highlights the fact that a virtual dtor is acceptable to use as the key function):

Recommendation for new platforms: when selecting the key function, also consider pure virtual destructors as candidates. Virtual destructors must be defined even if they are pure.

and per LLVM Coding Standards:

Provide a Virtual Method Anchor for Classes in Headers: If a class is defined in a header file and has a vtable (either it has virtual methods or it derives from classes with virtual methods), it must always have at least one out-of-line virtual method in the class. Without this, the compiler will copy the vtable and RTTI into every .o file that #includes the header, bloating .o file sizes and increasing link times.

and per lld documentation:

When a class has no non-pure, non-inline, virtual functions, there is no key function, and the compiler is forced to emit the vtable in every translation unit that references the class. In this case, it is emitted in a COMDAT section, which allows the linker to eliminate all duplicate copies. This is still wasteful in terms of object file size and link time, so it’s always advisable to ensure there is at least one eligible function that can serve as the key function.

I cannot find many references or materials regarding vtables, COMDAT groups, and their effect on linker performance. We could add as a third proposal to ignore/disable the -Wweak-vtable warning going forward, avoid defining any key function, and consider weak vtables to be an acceptable build performance overhead. However, the references above suggest to me that using a key function is still recommended, even if modern linkers may have improved the baseline performance level of handling weak vtables.

[kevinAlbs]

Avoid Ro5 boilerplate and out-of-line defaulted virtual destructors. (Accept this PR.)
Avoid ABI-stability-supporting a "useless" function in the vtable. (Reject this PR.)

I am slightly in favor of rejecting this PR. Adding Ro5 boilerplate seems like a small development cost. And adding a "useless" vtable function seems like a small runtime cost. Between the two, I'd rather have the small development cost.

That said, this is mostly new to me (and the references are appreciated).

[vector-of-bool]

I would be in favor of accepting the PR, with a code comment explaining the purpose of key_function. I don't think there will be a potential runtime cost to have key_function, but the Ro5 simplification may be worth having.

Alternatively, if it is decided to reject this PR: There should be a comment on the purpose of the out-of-line definition of ~exception().

[eramongodb]

Adding Ro5 boilerplate seems like a small development cost. And adding a "useless" vtable function seems like a small runtime cost. Between the two, I'd rather have the small development cost.

To clarify, this would be a small runtime memory cost (+1 vtable entry), but not a runtime performance cost (on the contrary, due to the inline dtor, we might see a minor performance improvement for non-LTO builds). According to this article (Feb 2023), the memory footprint of an additional vtable entry (for key_function()) is "almost negligable":

I think we can rightly expect that therefore a virtual method - due to the vtable - results in an increased binary size and we could also expect that any additional virtual method will add to the size of the binary as the vtable grows, but only a little bit. [...] We can see that the difference is small. In the previous articles, I used an array of 10,000 objects so that we don’t have to look into tiny differences. But in this case, it’s worth having a look at the exact size of the difference. It’s less than 200 bytes per 10,000 objects. To be more precise, the difference is 127 bytes, way less than one byte per object. It cannot have anything to do with the number of objects. It is only about the size of the vtable. It’s almost negligible. [...] It seems that the size of a vtable entry is about 48 bytes, at least on Apple Clang 15. [...] Having a virtual destructor ensures that everything necessary is instrumented for polymorphic behaviour which can make a huge difference in your binary size - and runtime performance. At the same time, adding another virtual function barely adds to the size of the binary. It’ll only mean an extra entry in your vtable.

Using .key_function() also comes with a small development cost (awareness of its impact on the stable ABI via the vtable entry).

[kevinAlbs]

Given the near negligible memory impact, no objections.