Skip to content

Conversation

poncito
Copy link
Contributor

@poncito poncito commented Oct 5, 2022

Implements a more efficient "break" when foldl-ing. I just reduce on an isbit type, that I call "BSONIndexUnsafe", instead of a BSONReader.

I test:

julia> buf = UInt8[]
       writer = BSONWriter(buf)
       writer[] = (;a=[1,2,3])
       close(writer)

       @btime begin
           reader = BSONReader($buf)
           reader["a"]["2"][Int64]
       end

       @btime begin
           reader = BSONReader($buf)
           reader["a"][3][Int64]
       end

Before:

17.576 ns (0 allocations: 0 bytes)
108.146 ns (2 allocations: 64 bytes)

and after

 17.618 ns (0 allocations: 0 bytes)
 17.911 ns (0 allocations: 0 bytes)

A few remarks:

  1. I'd like to remove the code duplication between:
  • function Transducers.__foldl__(rf, val, reader::BSONReader)
  • function Transducers.__foldl__(rf, val, indices::BSONIndices)
    but I don't know how, but it should be doable, no? Obviously the first method would rely on the second one.
  1. I'd like to remove the code duplication between:
  • Base.getindex(reader::BSONReader, target::Union{AbstractString, Symbol})
  • function Transducers.__foldl__(rf, val, indices::BSONIndices)
    That seems more tricky because of the name_len_and_match_ optimization.
  1. You will notice that the field el_type of BSONIndexUnsafe is of type Int. It should be a UInt8. But if I do that, the benchmark above runs in 60ns. I don't understand why my "fix" as any kind of impact. Any idea?

Thanks,

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2022

Codecov Report

Merging #26 (7998042) into master (ed7432c) will decrease coverage by 2.63%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
- Coverage   92.35%   89.71%   -2.64%     
==========================================
  Files          13       13              
  Lines         824      846      +22     
==========================================
- Hits          761      759       -2     
- Misses         63       87      +24     
Flag Coverage Δ
unittest 89.71% <94.73%> (-2.64%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/reader.jl 92.05% <94.73%> (-1.28%) ⬇️
src/writer.jl 88.82% <0.00%> (-8.46%) ⬇️
src/object_id.jl 86.04% <0.00%> (-6.98%) ⬇️
src/LightBSON.jl 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ancapdev
Copy link
Owner

ancapdev commented Oct 9, 2022

@poncito just letting you know I've seen the PR, but I'm quite busy at the moment and I think I need a little time to sit down and wrap my head around the changes here. Performance results look good though 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants