-
Notifications
You must be signed in to change notification settings - Fork 16
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
8c6741c
commit 8fde2d9
Showing
7 changed files
with
301 additions
and
291 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,3 +56,4 @@ class Model(Enum): | |
NICAM16_8S = 48 | ||
cams_europe = 49 | ||
cams_global = 50 | ||
cfsv2 = 51 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
8fde2d9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep in mind that for backward compatibility, you should always add new variables at the end, otherwise using an old client will decode the
enum
byte as a different variable!8fde2d9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 great data structure, I love it. Do you think we could suppress the
value: float
field in favor of thevalues: [float]
?Is there any scenario where we would find both value and values set @patrick-zippenfenig ? If we remove it we can use the same amount of bytes and reduce a little bit the generated code 😄
Here are some tests:
Both value and values in schema
test.fbs schema
Python script
Value set to
3.14
, values empty: 28 bytesValue unset, values set to
[3.14]
: 28 bytesValue set to
3.14
, values set to[3.14]
: 32 bytesJust values
test.fbs schema
Same python script, just remove the value stuff.
values set to
[3.14]
: 28 bytes8fde2d9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I will also add unit tests that confirm binary compatibility and will test for the last case in each enumeration.
Technically, it should reduce the indirection level. The value should be inlined into flatbuffers vtable while an array cannot. Your test was a bit surprising to me. Adding more attributes to the beginning of a table, but not using them, does increase the message size. The Flatbuffer docs, are a bit misleading.
Only setting
value
=> 32 bytes. I already suspected this behaviour, because the old format with attributes for each variable, had a lot of zeros inside.=> 20 bytes
Setting values to [3.14] => 28 bytes
This shows that 4 bytes are used for the array vtable offset and 4 bytes for the array size. Hence,
value: float
is (probably) inlined in the vtable while arrays use indirection. I am not sure yet what is the most efficient implementation. I will check first, how I can optimise the order of attributes to reduce the message size.