google / flatbuffers

FlatBuffers: Memory Efficient Serialization Library
https://flatbuffers.dev/
Apache License 2.0
23.28k stars 3.25k forks source link

Add ability to force-add a field #4231

Closed llchan closed 7 years ago

llchan commented 7 years ago

Sometimes it's desirable to ensure that a field is written to the buffer even if it's equal to the default. This matters when using mutation and you want to encourage reuse. There currently exists a buffer-wide force defaults flag, but not a per-field flag, or better yet, a per-add-call flag.

I'd like to discuss the possibility of adding something like this:

builder.add_foo(5, true);  // defaults to false

or

builder.force_add_foo(5);

Should be a very easy change that I can PR, just not sure what the API preference is.

llchan commented 7 years ago

My current workaround is to do it directly with this helper:

template <class T>
static inline void forceAddElement(fb::FlatBufferBuilder& fbb, fb::voffset_t field, T e)
{
  auto off = fbb.PushElement(e);
  fbb.TrackField(field, off);
}

This was lifted straight from flatbuffers.h, just with the default check removed.

Usage looks like this, which is fine, but I'd rather not have to know about the vtable constants:

forceAddElement(buf, Foo::VT_XYZ, 123);
aardappel commented 7 years ago

There is already a force_defaults argument to FlatBufferBuilder for this purpose.

llchan commented 7 years ago

I'm aware of that, but that applies at the buffer level. I'd like a little more granularity, because maybe certain fields should force defaults but not others. Seems pretty harmless to expose through the add functions unless I'm missing something? With a default argument of false, it would be a backwards-compatible change. If youre opposed to the change that's okay, I'm just curious why.

Sidenote: yes, some of this type of behavior can be achieved by converting some of these fields to structs and restructuring the messages, but that breaks backwards compatibility and I'm looking for a more incremental builder-side change that requires no client changes.

aardappel commented 7 years ago

I'm not necessarily against this kind of functionality, but the cost has to be considered, especially if not commonly used. E.g. adding an extra argument to all add functions does not sound great from that perspective, and neither does generating a lot of extra code (even if hidden behind a switch it has cost).

Actually, (at cost to you) you can already force defaults on a per field basis by simply calling FlatBufferBuilder::ForceDefaults before and after. Not elegant but it works.

llchan commented 7 years ago

Maybe I'm missing something, but I think the generated code would be pretty minimally-different:

// before
void add_id(uint32_t id) {
  fbb_.AddElement<uint32_t>(StringId::VT_ID, id, 0);
}

// after
void add_id(uint32_t id, bool force=false) {
  fbb_.AddElement<uint32_t>(StringId::VT_ID, id, 0, force);
}

Furthermore, I suspect that because these are all inline-able the compiler would optimize that flag away entirely in the default case (I havent verified this), so the performance cost should be small/zero.

Modifying the buffer-wide force defaults is a possible solution, and I do like that better than my current workaround because it doesnt involve vtable constants, but it involves unnecessary writes to the flag in memory rather than a flag in the call args that's likely to be optimized out anyways.

aardappel commented 7 years ago

Yes, but remember those extra args will be added to all fields for all users of FlatBuffers, none of whom so far have requested this functionality :) I know, small, but at this scale it matters.

llchan commented 7 years ago

Fair enough, your call. There are straightforward workarounds so nbd :)