tomjakubowski / nanopb

Automatically exported from code.google.com/p/nanopb
0 stars 0 forks source link

pb_message_set_to_defaults overhead when decoding #28

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
What steps will reproduce the problem?
1. create some message that has nested submessages. E.g.

message Leaf {
   required float someValue = 1;
}

message Root {
   required Leaf submessage = 1;
}

2. call pb_decode() for the Root structure.
3. pb_message_set_to_defaults is called first - the in walks recursively to all 
nested substructures and fields to set default values (like 0)
4. Then when going further step-by-step witin pb_decode(), submessage is being 
decoded (recursion) and pb_message_set_to_defaults is being called again (on 
submessage level) and then again it initializes fields of Leaf strucutre. 

So before field values are actually decoded and set, they are set to defaults 
twice.

If we would have message with 3-levels of nested submessages, this overhead 
would cause having given fields being initialized three times without a need.

Since it's minor overhead for messages without nesting, becomes large when 
dealing with heavily nested structures with larger strings or bytes fields 
(where memsets take a time).

What version of the product are you using? On what operating system?
nanopb-0.1.1. (but same code is in latest nanopb-0.1.5 version)
cross-compilation on Windows for ARM Cortex M4.
GCC-4.6 / devkitARM

Original issue reported on code.google.com by Adam.Cha...@gmail.com on 25 Aug 2012 at 8:29

GoogleCodeExporter commented 8 years ago
This issue was updated by revision f1d7640fe1be.

Original comment by Petteri.Aimonen on 26 Aug 2012 at 6:57

GoogleCodeExporter commented 8 years ago
Thanks for spotting this. I added pb_decode_noinit and modified 
pb_dec_submessage to use that, which should avoid the overhead.

I made pb_decode_noinit public on the grounds that some performance-conscious 
people might want to call memset(foo,0,sizeof(foo)) themselves if they need no 
default values.

Original comment by Petteri.Aimonen on 26 Aug 2012 at 6:59

GoogleCodeExporter commented 8 years ago
Gah, actually; I'll have to revert the previous one. It's more complex than I 
thought.

The submessages need to be initialized when pb_dec_submessage does its thing, 
because otherwise submessages inside an array would not receive the proper 
default values. Instead, pb_message_set_to_defaults might be able to skip the 
recursion, though then a submessage that is never received will be left 
uninitialized.

Original comment by Petteri.Aimonen on 26 Aug 2012 at 7:05

GoogleCodeExporter commented 8 years ago
This issue was updated by revision 160f02e4d0f8.

Original comment by Petteri.Aimonen on 26 Aug 2012 at 8:06

GoogleCodeExporter commented 8 years ago
Ok; now it should work properly and it is also backed by new testcases for the 
proper default fields initialization.

I think there should be no double-initialization happening now, though I didn't 
add testcases to guard for it (it would be just a performance problem anyway, 
difficult to test).

Original comment by Petteri.Aimonen on 26 Aug 2012 at 8:07

GoogleCodeExporter commented 8 years ago
Fix released in nanopb-0.1.6.

Original comment by Petteri.Aimonen on 3 Sep 2012 at 5:46