co-log icon indicating copy to clipboard operation
co-log copied to clipboard

Move from `Text` formatting to `Builder` (for `ByteString`)

Open chshersh opened this issue 7 years ago • 7 comments

Benchmark results show that ByteString.putStrLn works faster than Text.putStrLn so it probably makes sense to use ByteString as formatted representation of types like Message. Though, it looks like first we need to do more research in this area to be 100% sure.

chshersh avatar Jan 13 '19 11:01 chshersh

Improvements to benchmarks implemented by @vrom911 in #114 show that performance of encoding RichMessage to ByteString doesn't affect performance of logging that much. But maybe if we change formatting from Text to ByteString, this can increase performance in general.

chshersh avatar May 05 '19 04:05 chshersh

@vrom911 I noticed that in your PR #166 you worked with Builder directly instead of appending just Text. If my understanding is correct, appending Builders is much more efficient than appending Text. So instead of switching from Text to ByteString, we can turn to Builder (either for Text or ByteString). I have a feeling that if we use Builder directly, we can improve time formatting significantly. What do you think? What is that difficult to use Builder instead of Text, are there any gotchas you noticed while you were working on time formatting?

chshersh avatar Dec 30 '19 11:12 chshersh

Oh, that's a good idea! However, we should decide on the builder we choose. Even in the chronos library they have 2 different Builder types.

vrom911 avatar Dec 30 '19 12:12 vrom911

My gut feeling says that ByteString builder is better. Usually IO with ByteString is much more reliable and faster. And there's even a function that can output a Builder directly to Handly so we can avoid conversion to ByteString completely if we create LogAction IO Builder!

  • https://hackage.haskell.org/package/bytestring-0.10.10.0/docs/Data-ByteString-Builder.html#v:hPutBuilder

chshersh avatar Dec 30 '19 12:12 chshersh

In that case, I can propose the following plan:

  • [ ] Implement logBuilder* :: LogAction m Builder family of functions
  • [ ] Change Msg and SimpleMsg types to store Builder inside instead of Text
  • [ ] Change arguments of all log* functions to take Builder instead of Text. This is a bit controversial. But since Builder has IsString and Semigroup instances, it still should be possible to use string literals and show functions to create messages
  • [ ] Change all formatting functions to return Builder instead of Text. If understand correctly, only chronos part can be a bit difficult due to absence of some builders in there (@vrom911, correct me if I'm wrong here)
  • [ ] Add benchmarks with Builder and use logBuilder action for msg and rerun benchmarks

Does this sound good?

chshersh avatar Jan 04 '20 15:01 chshersh

Yes, sounds great!

I don't think that chronos would be the most controversial, we can always implement what we need in co-log and propose to add it to the chronos library later.

vrom911 avatar Jan 04 '20 16:01 vrom911

To be more backwards-compatible in the new version, we can replace only internal types for formatting with Builder and keep functions that take Text. Those functions will be deprecated in the future, once we move to the structured logging.

chshersh avatar Aug 21 '20 16:08 chshersh