Refactor cljam.io.util.bin

[alumi]

Summary

Refactored cljam.io.util.bin

Changes

• Rearrange functions
• Add docstrings
• Add tests

Tests

• lein check ✅
• lein test :all ✅
• lein cloverage ✅
• lein eastwood ✅
• lein cljfmt check ✅

[niyarin]

LGTM:+1:

[codecov]

Codecov Report

Merging #201 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   86.91%   86.94%   +0.02%     
==========================================
  Files          77       77              
  Lines        6267     6266       -1     
  Branches      520      518       -2     
==========================================
+ Hits         5447     5448       +1     
  Misses        300      300              
+ Partials      520      518       -2     

Impacted Files	Coverage Δ
src/cljam/io/util/bin.clj	98.07% <100.00%> (+3.73%)	⬆️

---

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 84a535d...aa62440. Read the comment docs.

[athos]

Thanks for refactoring! 💪

Just to confirm, memoization for reg->bins was dropped, but is it intended?
If so (probably so), could you elaborate a little more on the reason just for future reference? It messed up the performance improvement by unboxed function calls? You were concerned about its memory consumption? Or for any other reason?

[alumi]

Just to confirm, memoization for reg->bins was dropped, but is it intended?
If so (probably so), could you elaborate a little more on the reason just for future reference? It messed up the performance improvement by unboxed function calls? You were concerned about its memory consumption? Or for any other reason?

Ah, sorry I forgot to mention that 🙇
The changes to reg->bins are intentional.

• I fixed it to use transduce to speed it up by 1.5x-2x.
• I decided to remove the memoize because it's not repeatedly called and it likely to have a high cache miss rate (not well tested). reg->bins* was fast (~10 μs) enough by itself compared to actual file I/O ops. As you said, this will benefit reducing CPU overhead and memory leak.
• I made it public to balance with reg->bin.

[athos]

Thank you for describing your decisions behind the refinements! They all sound reasonable to me 🙆‍♂️

I decided to remove the memoize because it's not repeatedly called and it likely to have a high cache miss rate (not well tested).

Yeah, I can imagine that. The (beg, end) space in general might be too sparse for memoization to work effectively (which depends heavily on how reg->bins is used, though). But even without built-in memoization, users can memoize the function on their own if they really need.

[alumi]

Thank you for reviewing! 😸 @niyarin @athos