well-typed / cborg

Binary serialisation in the CBOR format
https://hackage.haskell.org/package/cborg
188 stars 86 forks source link

UTCTime serialisation slow #51

Closed teh closed 7 years ago

teh commented 8 years ago

For testing I replaced my Binary instances with Serialise from this package and serialization performance was 10-20x slower. I narrowed it down to UTCTime. Replacing that with a dummy empty serialiser speeds up my code from 3 seconds to 150 milliseconds.

Instance in question:

https://github.com/well-typed/binary-serialise-cbor/blob/9528877a4d85642be787c05efee39d2b3e0e078e/Data/Binary/Serialise/CBOR/Class.hs#L402

teh commented 8 years ago

The actual format is part of the standard: https://tools.ietf.org/html/rfc7049#section-2.4.1 so there's probably no wiggle room in changing that to the nicer UTCTime day/time format. I tried writing a faster encoder but time conversions are slow. Will probably have to newtype with custom serialise :/

arianvp commented 8 years ago

You could newtype it and use the same serialisation format as the binary package?

thoughtpolice commented 8 years ago

It'd be interesting to see where you got your Binary instances from - you took an Orphan I imagine (or newtyped it) something like this I assume?

instance Binary UTCTime where
  put (UTCTime a b) = put a >> put b
  get = UTCTime <$> get <*> get

instance Binary Day where
  put (ModifiedJulianDay d) = put d
  get = ModifiedJulianDay <$> get

instance Binary DiffTime where
  put = put . fromEnum
  get = toEnum <$> get

...

or whatever? I can definitely believe that the time formatting functions are simply slow no matter what, since they have to roundtrip through String and probably a trillion other things.

Second, if you can file a synthetic benchmark or something with our criterion setup, that'd be really nice! It'd be interesting to investigate this (it's a bit hard to say exactly what could be improved without looking at a profile).

If this is an insurmountable problem, it would probably be OK to provide another function like reallyFastUTCEncoding :: UTCTime -> Encoding that does something similar, and just mention it very prominently in the tutorial and documentation that any time a UTCTime is needed, you should carefully consider which one you need. This is a really horrible workaround, though, and gets very unusable if you ever want some function overloaded on Serialise (you can't just call encode, so you'd have to take an Encoding instead or something). FWIW: A literal translation of any binary instance should always be faster, basically without exception - but this is a case where one of our instances has to be different from upstream for the stated behavior.

That said, providing the canonical instance is, IMO correct (we don't want orphans for this). It sucks that it's so slow, though.

teh commented 8 years ago

@thoughtpolice - yea, that's pretty close: https://hackage.haskell.org/package/binary-orphans-0.1.4.0/docs/src/Data.Binary.Orphans.html

I'm trying to add a UTCTime benchmark but can't get the benchmark to compile. GHC (7.10.3) has been stuck for over an hour now compiling PkgAesonGeneric - have you seen this?

[16 of 28] Compiling Macro.PkgAesonGeneric ( bench/Macro/PkgAesonGeneric.hs, dist/build/vs-other-libs/vs-other-libs-tmp/Macro/PkgAesonGeneric.o )
23247 me       20   0 4878848 3.542g  22980 S 310.3 46.1  15:06.19 ghc                                                                                       
teh commented 8 years ago

Removed that benchmark for now while testing. See #52. I'm totally unclear on whether that benchmark actually does what I want (with force) ..

thoughtpolice commented 8 years ago

The benchmark compilation performance is a known problem - see #33.

I'll take a closer look at #52. Ugh, I need to get our 32 bit support online...

thoughtpolice commented 8 years ago

Oh yes, and Generic might be especially bad due to some GHC bugs. I'll test for a bit.

dcoutts commented 8 years ago

Sigh. It's unfortunate that it's so slow. So we have two choices:

  1. use the standard CBOR representation for greater compatibility (e.g.cbor2json tools will know how to render them) and try and make the parser/printer faster, or
  2. go for a different representation in terms of the day + time of day. Of course we only have to handle the one standard date/time format, unlike the time lib with its general format string interpretation.
adamgundry commented 8 years ago

I suspect this is a problem for our application as well, which serializes large datasets full of UTCTimes. Would it be worth working on a higher-performance RFC 3339 parser/printer (if such a thing doesn't exist already)? That might be a nice little project that could be useful in other contexts.

Alternatively, I notice that the CBOR standard permits POSIX-style timestamps as well (tag 1). Though those don't seem to be supported by the Serialise UTCTime instance...

adamgundry commented 8 years ago

Looks like aeson might have some useful prior art.

thoughtpolice commented 8 years ago

Right, so Aeson uses ISO-8601 for encoding UTCTime, which is also exactly what CBOR uses. The writer bit should be pretty easy to steal, since it already uses a Builder to write out the encoded format. But the parser will be tricker as we'll have to eliminate the attoparsec dependency, I think. But the initial results of reusing that code seem promising at a glance.

bgamari commented 7 years ago

We are going to move to a new, more reasonable encoding for the UTCTime instance for the release. However, this can be done in a backwards compatible way as it is tagged.

One possible encoding is two integers: days since the 0 epoch and picoseconds.

thoughtpolice commented 7 years ago

It looks like this will go into 0.2 regardless at this rate, so I'm queuing this for the initial release.

bgamari commented 7 years ago

I have submitted an IANA request for the new tags but it will take some time for the process to take it course. I don't think we should hold the release for this.

thoughtpolice commented 7 years ago

Fixed by d093bb6e65f1bbac1d1319201ad09ae2b604b3b4.

thoughtpolice commented 7 years ago

Errr, actually fixed by cad4c726cd3bcf231e88e4b2d2129a33f83b1461! (Ben accidentally force pushed over the last commit)