Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

flatbuf #403

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

flatbuf #403

wants to merge 8 commits into from

Conversation

tbfleming
Copy link
Member

Depends on #330

@tbfleming
Copy link
Member Author

tbfleming commented Aug 17, 2021

Issues I've found so far:

  • For API convenience, and the original author's assumption that memcpy hurts performance, it exposes data as cast pointers or references.

    • Relies on P0593R6 (accepted as a DR resolution to 17 and 20), otherwise it's UB since object lifetimes never begin.
    • P0593R6 places requirements on types. e.g. non-trivial flat<std::variant<Ts...>>::flat(), which only exists for the static_asserts, disqualifies it. Fixable by either removing the constructor, or by adding a trivial copy constructor.
    • Requires care to ensure "3.2. When to create objects" happens.
    • P0593R6 doesn't allow strict-aliasing violations. Proxy objects alias with with the stored types. Since the proxy objects have no data members, we're hopefully ok even though we are UB.
  • Doesn't align data

    • Option 1: keep unaligned and keep cast pointers and references. Since there doesn't seem to be a gcc or clang attribute to mark pointers as unaligned, this is playing with fire.
    • Option 2: memcpy on every access. Additional wrappers (e.g. flat<uint32_t>) can reduce the harm to API convenience.
    • Option 3: insert padding to align all numeric types to their size, so will meet the alignment requirements of existing architectures. Where the max alignment isn't known in advance (variants), use a 16-byte (uint128_t) alignment. Hope (and static_assert) we never encounter a future uint256_t.
  • Tuples and non-reflected structs

    • As long as these pass a is_trivially_copyable check, it stores these raw then uses pointer and reference casts.
    • Problem 1: struct as a whole is accessed unaligned
    • Problem 2: padding between fields varies with architecture. Ideally we could require these structs be packed using gcc attributes, but I don't think this is checkable. We can't modify tuple to be packed.
    • Problem 3: tuple layout order isn't guaranteed by the ISO standard
    • Problem 4: since typical tuple implementations use inheritance, there's additional weirdness with padding
    • Best option is probably to remove support.
  • Not a problem per se, but a design tradeoff: variants use hashed type names for the discriminator.

    • Advantage: multiple people adding multiple new types won't produce ambiguities, as long as the new type names differ and the hashes don't collide.
    • Disadvantage: we encountered a hash collision when we attempted this approach for long action names
    • Disadvantage: changing the name of a type breaks binary compatibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant