Replace Netty based RFC 1123 date encoding (#3527)#3550
Conversation
✅ Deploy Preview for zio-http ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Here is an efficient implementation of parsing/formatting for different timestamp formats including RFC 1123. Most performance tricks were ported from jsoniter-scala routines for JSON parsing and serialization of different data time values. Java's |
|
|
||
| def decodeDate(date: String): Option[ZonedDateTime] = { | ||
| try { | ||
| if (date.head.isUpper && date.charAt(3) == ',' && date.charAt(4) == ' ') |
There was a problem hiding this comment.
date.head throws if date is empty.
Should we maybe prefer to do:
if (date.isEmpty) None
else if (date.charAt(0).isUppe && ...)
...?
Probably a good change for performances
There was a problem hiding this comment.
I checked instead, that length is 28 or 29 since only these are valid.
Theoretically 27 is valid (year before 1k) But I think it is okay to ignore
| private def decodeRFC1123(date: String): Option[ZonedDateTime] = { | ||
| { | ||
| // TODO consider trie-hard approach | ||
| val c0 = date.head |
There was a problem hiding this comment.
Prefer date.charAt(0)? It avoids an if (string.isEmpty) check present in the .head method
Related: https://github.com/zio/zio-http/pull/3550/files#r2155802128
459b391 to
d5f8ea8
Compare
|
@guizmaii Implemented some benchmarks Before your suggested changes After The invalid case has explicitly strings with invalid length. |
a7cfa72 to
7196299
Compare
|
Cool job! Although why not make these changes in Netty code for everyone to benefit? |
|
|
||
| def decodeDate(date: String): Option[ZonedDateTime] = { | ||
| try { | ||
| if (date.length != 29 && date.length != 28) return None |
There was a problem hiding this comment.
@plokhotnyuk Is there any cost of calling date.length twice? Or the compiler and/or JIT will optimise this? 🤔
There was a problem hiding this comment.
String length is just a call to an internal field of the string impl. I don't think there is a real cost for this call.
I can be wrong but I think the idea is probably to have this code in pure Scala so that it compiles to JS and Native and reduces the dependency on Netty |
7196299 to
cac56dd
Compare
| encodeRFC1123(date) | ||
|
|
||
| def decodeDate(date: String): Option[ZonedDateTime] = { | ||
| if (date.length != 29 && date.length != 28) return None |
fixes #3527
/claim #3527