Open adrianschultz opened 1 year ago
Investigations on our side regarding the traffic participant called "OSI-Pedestrian". Thanks to @FabianPfeufferCAP and @tbleher !!
For the sake of completeness I am adding further links to reports here: 1) AVL report 2) Persival report
In addition, check out our Application report
Everyone could give a "thumbs up" on this issue to raise awareness for this bug https://github.com/google/flatbuffers/issues/7109
@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?
@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.
@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 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 :)
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
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
Copying of stuff (e.g. GlobalGroundTruth or SensorViewConfiguration) is now moved to https://github.com/OpenSimulationInterface/open-simulation-interface/issues/703
Benchmark results/presentation slides - Flatbuf struct vs. table vs. protobuf based on https://github.com/OpenSimulationInterface/osi-sensor-model-packaging/tree/feature/flatbuffers_examples
@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?
@jdsika and @pmai Was this PR already created? - If not, I would start it now :-)
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.
As discussed in the CCB meeting today, @pmai is preparing a PR for this. Thank you!
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.
Zoomed in on max. 20.000 objects:
Zoomed in on max. 10.000 objects:
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.
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.
As it was a question in todays CCB meeting (2023-09-11): Results - OSI Project Questionnaire - 23-06-12 - Protobuf vs. Flatbuffer
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:
Cons:
General remarks: