zio-http icon indicating copy to clipboard operation
zio-http copied to clipboard

Indexed fields cookie changes

Open jgoday opened this issue 3 years ago • 3 comments

This PR tries to implement a fields indexed cookie view to avoid allocation memory whenever possible. It was suggested by @tusharmath in a previous PR (#576).

  • It defines a Cookie trait with the basic properties and method signatures.
  • CookieImpl as a case class to instantiate a cookie with all of his fields when necessary. Cookie object apply method invokes this case class.
  • A CookieView that takes a raw string cookie value. It postpones memory allocation (of each field) until needed.
  • CookieBuilder trait to allow mutate a cookie.

I'm not really sure about naming anyway, CookieImpl does not convince me at all.

This are the CookieDecodeBenchmark results (it's not very fair to compare them ...).

Main branch:

[info] Result "zhttp.benchmarks.CookieDecodeBenchmark.benchmarkApp":
[info]   1910631,365 ±(99.9%) 88274,142 ops/s [Average]
[info]   (min, avg, max) = (1786687,328, 1910631,365, 2041812,681), stdev = 82571,687
[info]   CI (99.9%): [1822357,224, 1998905,507] (assumes normal distribution)
[info] # Run complete. Total time: 00:05:03

Indexed field cookie:

[info] Result "zhttp.benchmarks.CookieDecodeBenchmark.benchmarkApp":
[info]   4714328,763 ±(99.9%) 245711,977 ops/s [Average]
[info]   (min, avg, max) = (4501769,424, 4714328,763, 5239183,243), stdev = 229839,136
[info]   CI (99.9%): [4468616,786, 4960040,740] (assumes normal distribution)
[info] # Run complete. Total time: 00:05:03

jgoday avatar Jan 30 '22 20:01 jgoday

Codecov Report

Merging #924 (fff12c5) into main (61e57a0) will increase coverage by 0.41%. The diff coverage is 79.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #924      +/-   ##
==========================================
+ Coverage   56.92%   57.33%   +0.41%     
==========================================
  Files          73       73              
  Lines        2259     2269      +10     
  Branches       66       70       +4     
==========================================
+ Hits         1286     1301      +15     
+ Misses        973      968       -5     
Impacted Files Coverage Δ
zio-http/src/main/scala/zhttp/http/Cookie.scala 78.10% <79.36%> (+5.66%) :arrow_up:

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 61e57a0...fff12c5. Read the comment docs.

codecov-commenter avatar Jan 30 '22 20:01 codecov-commenter

@jgoday Let me know if you need help here. Thanks once again for your contributions!

tusharmath avatar Feb 11 '22 11:02 tusharmath

@jgoday Let me know if you need help here. Thanks once again for your contributions!

Hi @tusharmath, what do you think about this implementation ?

jgoday avatar Feb 17 '22 20:02 jgoday

Cookie rewrite #1414 internally uses netty's decoder/encoder. I don't think ZIO Http needs to worry about it anymore.

Thanks @jgoday ! Closing this now.

tusharmath avatar Sep 07 '22 10:09 tusharmath