Drvi / ProtocolBuffers.jl

4 stars 0 forks source link

Optimization pass #7

Closed Drvi closed 2 years ago

Drvi commented 2 years ago

This started as an effort to make encoding and decoding fast to IOBuffers and IOStreams and the associated buffered IO packages BufferedStreams.jl and TranscodingStreams.jl.

For IOBuffers, we got rid of the temporary IOBuffers used to encode length delimited fields; we now reserve space for the length encode the data past it, encode the length and shift the data back if we didn't use all the space needed for the length. We also try harder to work around https://discourse.julialang.org/t/allocation-due-to-noinline-for-unsafe-read-and-unsafe-write-in-io-jl/69421, when julia allocates when writing numbers to GC-root them (IIUC). We still hit these for IOStream, but are able to avoid these allocs when using IOBuffer/BufferedStreams/TranscodingStreams as those use in memory buffers.

For IOStreams had to use a different strategy to encode length delimited fields. We introduced a new method that gets generated for each message,_encoded_size, which returns the encoded size of a struct (taking into account that default values are not going to be encoded, lengths, tags etc).

Apart from that, I also increased test coverage in which revealed what was missing for supporting groups properly + changed how we parametrize OneOf fields from:

struct A{T1<:Union{Int,String}}
   oneof_field::Union{Nothing,OneOf{T1}}
end

to

struct B{T1<:Union{Nothing,OneOf{<:Union{Int,String}}}}
   oneof_field::T1
end

because the then the constructor doesn’t get confused like this:

julia> A(nothing)
ERROR: UndefVarError: T1 not defined
Stacktrace:
 [1] A(oneof_field::Nothing)
   @ Main ./REPL[2]:2
 [2] top-level scope
   @ REPL[5]:1

julia> B(nothing)
B{Nothing}(nothing)

But in the future, the default will change to generate the following definition and not parametrize on the type:

struct C
    oneof_field::Union{Nothing,OneOf{<:Union{Int,String}}}
end
codecov-commenter commented 2 years ago

Codecov Report

Merging #7 (191094e) into master (bbb094a) will increase coverage by 1.15%. The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
+ Coverage   88.32%   89.48%   +1.15%     
==========================================
  Files          21       22       +1     
  Lines        2399     2502     +103     
==========================================
+ Hits         2119     2239     +120     
+ Misses        280      263      -17     
Impacted Files Coverage Δ
src/ProtocolBuffers.jl 85.71% <0.00%> (-6.60%) :arrow_down:
src/codec/decode.jl 86.32% <71.42%> (+8.94%) :arrow_up:
src/codec/encoded_size.jl 81.81% <81.81%> (ø)
src/codegen/encode_methods.jl 91.20% <92.68%> (+0.46%) :arrow_up:
src/codegen/toplevel_definitions.jl 96.82% <93.33%> (+0.18%) :arrow_up:
src/codec/encode.jl 98.64% <96.25%> (+2.85%) :arrow_up:
src/codec/Codecs.jl 92.30% <100.00%> (+3.41%) :arrow_up:
src/codec/vbyte.jl 98.80% <100.00%> (+0.02%) :arrow_up:
src/codegen/decode_methods.jl 98.83% <100.00%> (ø)
src/codegen/modules.jl 83.33% <100.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bbb094a...191094e. Read the comment docs.