open-simulation-interface icon indicating copy to clipboard operation
open-simulation-interface copied to clipboard

Flatbuffer investigations collector

Open adrianschultz opened this issue 3 years ago • 18 comments

Please add your learnings and results here about your flatbuffer investigations, so that we can have a detailed discussion in the performance and packaging WG.

Pros:

  • faster than protobuffers -> especially on sensor model side (de-serialization)

Cons:

  • more complex code especially for serialization -> Are the improvement gains worth the refactoring effort for existing code?

General remarks:

  • Are there other possibilties to acchieve better performance?
  • Flexibility vs performance: Do we want to keep OSI as flexible as it is right now and life with performance limitations or not?

adrianschultz avatar Nov 21 '22 07:11 adrianschultz

Investigations on our side regarding the traffic participant called "OSI-Pedestrian". Thanks to @FabianPfeufferCAP and @tbleher !!

OsiPedestrian-Flatbuffer-Protobuf-Comparison.pdf

ThomasNaderBMW avatar Dec 15 '22 11:12 ThomasNaderBMW

For the sake of completeness I am adding further links to reports here:

  1. AVL report
  2. Persival report

jdsika avatar Jan 19 '23 10:01 jdsika

In addition, check out our Application report

PhRosenberger avatar Jan 19 '23 11:01 PhRosenberger

Everyone could give a "thumbs up" on this issue to raise awareness for this bug https://github.com/google/flatbuffers/issues/7109

jdsika avatar Jan 19 '23 14:01 jdsika

@pmai should we just create a PR that puts nested ENUM definitions at the beginning of the message definition? I do not think that this would break backwards compatibility?

grafik

jdsika avatar Jan 19 '23 14:01 jdsika

@PhRosenberger I think we should open a feature request in the Flatbuffer GitHub and ask for the feature? What do you think?

Flutbuffers does not come with a deep copy function, so there is no easy way to copy table or entire buffers to a different buffer. With OSI it is currently common practice to copy the SensorView input of a sensor model to the SensorData output. As there is no standard function to do this with Flatbuffers, one has to be custom written.

jdsika avatar Jan 19 '23 14:01 jdsika

@PhRosenberger I think we should open a feature request in the Flatbuffer GitHub and ask for the feature? What do you think?

Flutbuffers does not come with a deep copy function, so there is no easy way to copy table or entire buffers to a different buffer. With OSI it is currently common practice to copy the SensorView input of a sensor model to the SensorData output. As there is no standard function to do this with Flatbuffers, one has to be custom written.

Yes we can do that, but I am a bit disappointment by the response to our issue over there, so I am not sure if we can expect it to actually happen. If we get some feedback, it would be nice to discuss such a feature with the google team behind FlatBuffers.

PhRosenberger avatar Jan 19 '23 15:01 PhRosenberger

@PhRosenberger I think we should open a feature request in the Flatbuffer GitHub and ask for the feature? What do you think? Flutbuffers does not come with a deep copy function, so there is no easy way to copy table or entire buffers to a different buffer. With OSI it is currently common practice to copy the SensorView input of a sensor model to the SensorData output. As there is no standard function to do this with Flatbuffers, one has to be custom written.

Yes we can do that, but I am a bit disappointment by the response to our issue over there, so I am not sure if we can expect it to actually happen. If we get some feedback, it would be nice to discuss such a feature with the google team behind FlatBuffers.

Yes, I can understand this feeling. Let's just keep going and do the feature request. If we all start commenting on it we might get things moving :)

jdsika avatar Jan 19 '23 15:01 jdsika

Yes, I can understand this feeling. Let's just keep going and do the feature request. If we all start commenting on it we might get things moving :)

I just created such an issue, so we will know their thoughts on this soon, hopefully: https://github.com/google/flatbuffers/issues/7798

PhRosenberger avatar Jan 23 '23 08:01 PhRosenberger

We should discuss the concept of nested flatbuffers, e.g. nesting SensorView and then nesting GlobalGroundTruth inside of the SensorView, as suggested by https://github.com/google/flatbuffers/issues/7798#issuecomment-1401363501

PhRosenberger avatar Jan 24 '23 08:01 PhRosenberger

Copying of stuff (e.g. GlobalGroundTruth or SensorViewConfiguration) is now moved to https://github.com/OpenSimulationInterface/open-simulation-interface/issues/703

PhRosenberger avatar Feb 09 '23 11:02 PhRosenberger

Benchmark results/presentation slides - Flatbuf struct vs. table vs. protobuf based on https://github.com/OpenSimulationInterface/osi-sensor-model-packaging/tree/feature/flatbuffers_examples

presentation-flatbuf-comparison.pdf

thomassedlmayer avatar Apr 20 '23 10:04 thomassedlmayer

@pmai should we just create a PR that puts nested ENUM definitions at the beginning of the message definition? I do not think that this would break backwards compatibility?

grafik

@jdsika and @pmai Was this PR already created? - If not, I would start it now :-)

PhRosenberger avatar Apr 24 '23 11:04 PhRosenberger

As discussed in the performance and packaging workgroup last week, we will now start a questionnaire for the whole OSI project to get an overview on the usage of OSI and if there is a need for switching to Flatbuffers or not.

PhRosenberger avatar May 08 '23 07:05 PhRosenberger

As discussed in the CCB meeting today, @pmai is preparing a PR for this. Thank you!

PhRosenberger avatar May 08 '23 09:05 PhRosenberger

I finally managed to run the benchmark of the BMW OSI Pedestrian code (https://github.com/OpenSimulationInterface/open-simulation-interface/issues/691#issuecomment-1352900872) with more objects (up to 79.000). I noticed that the mutate function was used for initially filling the flatbuffer structs. According to the flatbuffer tutorial mutate is not recommended as the default way of setting flatbuffer data (see https://flatbuffers.dev/flatbuffers_guide_tutorial.html; section "Mutating FlatBuffers"). Though, it did not significantly improve performance when changing the following structure:

auto size3d = osi3::Dimension3d();
size3d.mutate_height(size.height());
size3d.mutate_width(size.width());
size3d.mutate_length(size.length());

to:

auto size3d = osi3::Dimension3d(size.height(), size.width(), size.length());

for all structs.

Benchmark results:

Figure_1

Zoomed in on max. 20.000 objects:

Figure_2

Zoomed in on max. 10.000 objects:

Figure_3

I did not run the benchmark multiple times as it was done for the initial BMW report, so I do not have any error/variance values. But the trend for up to 3000 objects is relatively similar to the BMW report. I think it should give a good enough general idea on the performance improvements.

thomassedlmayer avatar May 26 '23 10:05 thomassedlmayer

Great to see these results @thomassedlmayer . Thanks for spending the efforts on reproducing it! It looks way less linear than in the BMW report for the lower numbers of objects.

PhRosenberger avatar May 26 '23 10:05 PhRosenberger

As it was a question in todays CCB meeting (2023-09-11): Results - OSI Project Questionnaire - 23-06-12 - Protobuf vs. Flatbuffer

florianwallerer avatar Sep 11 '23 12:09 florianwallerer