Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.20%. Comparing base (4e93fa3) to head (8f370b5).

@@             Coverage Diff              @@
##               main     #367      +/-   ##
============================================
+ Coverage     70.17%   70.20%   +0.02%     
- Complexity     1070     1072       +2     
============================================
  Files           306      306              
  Lines         12337    12322      -15     
  Branches       1179     1178       -1     
============================================
- Hits           8658     8651       -7     
+ Misses         3200     3179      -21     
- Partials        479      492      +13     

So to sum up, this is a non-breaking change, we could compress better (by ~7%) if we re-compress the full blob each time we attempt to append a block; but doing so is too slow so you propose to keep the original method, that would give a "preliminary result", and change the result behind the scenes between calls?

Wouldn't this have some side effects @jpnovais ? (i.e basically the compressor could say "we compressed block 1, it takes N Bytes, current blob is now at 100kB", then recompress it with more context and update the internal state to "current blob is 98kB" without notifying the coordinator. ).

Re implementation, before introducing an async optimizer, I'ld prefer to understand perf constraints better; i.e. how long it takes now, how long it would take if we recompress all the blob at each append, and within what limit we need operate . i.e. if we say the compressor could take as much as XXXms , then we may just want to have a simpler cleaner code and kill this async pattern.

My input on this optimization is:

Context:

• The coordinator does not actively keep track of the current blob size, that is the responsibility of the compressor, which is aware of blob limit.
• Coordonator calls fun CanWrite(data: ByteArray, data_len: Int): Boolean before calling Write, if returns false then coordinator knows the blob is full and starts a new one

My take based on the above:

• As mentioned, Asyc can be problematic so I would avoid it. Also, I think we don't need it here.
• My suggestion: when coordinator calls CanWrite before returning false, the compressor would try to perform the full compression an see if that extra blob fits.
  • pros:
    • lazy computation - it only does the "full re-compression" when it reaches the limit;
    • 100% transparent to the coordinator and keeps same API. This leaves room for further internal code performance optimizations ;

@Tabaie @gbotrel WDYT?

Yep that would make less CPU use to do it only when full . But this last call to "CanWrite" may be 10x slower than the previous calls.
So what are the bounds perf-wise we need to operate in? If a call to CanWrite takes 500ms is that acceptable? (not saying it will, just want a rough order of magnitude of bound)

So what are the bounds perf-wise we need to operate in? If a call to CanWrite takes 500ms is that acceptable? (not saying it will, just want a rough order of magnitude of bound)

It's ok to have a call to CanWrite that takes 500ms at the of the blob, as long as the preceding calls are not affected timewise.

I agree, doing it synchronously at the end is a good idea. In fact it's similar to the "no compress" logic we already have.