Performance: Cookie decoder improvements #576

jgoday · 2021-11-26T20:47:34Z

Reduce Cookie case class allocations in Cookie.decodeResponseCookie.
Right now, it allocates a new Cookie instance with each field (using Cookie.copy through several methods).
For example,
it allocates 3 instances for "SUID=123;httponly=true;expires=2007-12-03T10:15:30.00Z".

This change parses each field separately and then creates one single Cookie instance.

This simple jhm benchmark show the two approaches

package sample

import java.util.concurrent.TimeUnit
import org.openjdk.jmh.annotations._
import zhttp.http.Cookie

@State(Scope.Thread)
@BenchmarkMode(Array(Mode.Throughput))
@OutputTimeUnit(TimeUnit.MICROSECONDS)
class CookieDecode {
  @Benchmark
  def benchmarkDecode(): Unit = {
    val value="SUID=123;httponly=true;expires=2007-12-03T10:15:30.00Z"
    val _ = Cookie.decodeResponseCookie(value)
    ()
  }
}

Parsing and copying Cookie case class


# Warmup Iteration   1: 0,141 ops/us
[info] # Warmup Iteration   2: 0,151 ops/us
[info] # Warmup Iteration   3: 0,206 ops/us
[info] Iteration   1: 0,217 ops/us
[info] Iteration   2: 0,168 ops/us
[info] Iteration   3: 0,147 ops/us
[info] Result "sample.CookieDecode.benchmarkDecode":
[info]   0,177 ┬▒(99.9%) 0,653 ops/us [Average]
[info]   (min, avg, max) = (0,147, 0,177, 0,217), stdev = 0,036
[info]   CI (99.9%): [? 0, 0,831] (assumes normal distribution)
[info] # Run complete. Total time: 00:01:01

Parsing fields with mutable vars

[info] # Warmup Iteration   1: 0,340 ops/us
[info] # Warmup Iteration   2: 0,375 ops/us
[info] # Warmup Iteration   3: 0,354 ops/us
[info] Iteration   1: 0,369 ops/us
[info] Iteration   2: 0,375 ops/us
[info] Iteration   3: 0,349 ops/us
[info] Result "sample.CookieDecode.benchmarkDecode":
[info]   0,364 ┬▒(99.9%) 0,244 ops/us [Average]
[info]   (min, avg, max) = (0,349, 0,364, 0,375), stdev = 0,013
[info]   CI (99.9%): [0,120, 0,608] (assumes normal distribution)
[info] # Run complete. Total time: 00:01:00

codecov-commenter · 2021-11-26T22:10:48Z

Codecov Report

Merging #576 (58ee1bf) into main (57d6126) will decrease coverage by 8.02%.
The diff coverage is 81.94%.

@@            Coverage Diff             @@
##             main     #576      +/-   ##
==========================================
- Coverage   57.36%   49.34%   -8.03%     
==========================================
  Files          66       68       +2     
  Lines        1466     2061     +595     
  Branches       55       53       -2     
==========================================
+ Hits          841     1017     +176     
- Misses        625     1044     +419

Impacted Files	Coverage Δ
zio-http/src/main/scala/zhttp/http/Cookie.scala	`63.47% <81.94%> (+9.06%)`	⬆️
.../scala/zhttp/service/client/ClientSSLHandler.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...cala/zhttp/service/server/OptionalSSLHandler.scala	`0.00% <0.00%> (-78.58%)`	⬇️
.../scala/zhttp/service/server/ServerSSLHandler.scala	`16.66% <0.00%> (-66.67%)`	⬇️
...cala/zhttp/service/server/HttpOnHttpsHandler.scala	`0.00% <0.00%> (-61.54%)`	⬇️
...ttp/src/main/scala/zhttp/service/HttpRuntime.scala	`35.71% <0.00%> (-60.72%)`	⬇️
zio-http/src/main/scala/zhttp/http/HttpData.scala	`29.41% <0.00%> (-32.13%)`	⬇️
...la/zhttp/service/client/ClientInboundHandler.scala	`71.42% <0.00%> (-28.58%)`	⬇️
...io-http/src/main/scala/zhttp/service/Handler.scala	`65.78% <0.00%> (-28.19%)`	⬇️
...http/service/client/ClientChannelInitializer.scala	`85.71% <0.00%> (-14.29%)`	⬇️
... and 57 more

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 57d6126...58ee1bf. Read the comment docs.

tusharmath · 2021-11-27T05:13:07Z

zio-http/src/main/scala/zhttp/http/Cookie.scala

+        ),
+      )
+    } else {
+      None


This is great, you have almost doubled the perf!

You can further reduce allocations by —

Get rid of all the Option and use null instead.

Every map operation is creating a new collection (more memory), use a for/while loop if possible

Get rid of interim Function and Tuple also.

Don't allocate the String inside the method, pull them outside so that thy are allocated only once.

NOTE: You can have a method called - unsafeDecodeResponseCookie that returns a null or a Cookie for performance and make it package private. Then also have a decodeResponseCookie which calls unsafeDecodeResponseCookie and wraps it in an Option

Made some changes to avoid string split and some allocations.

Marked internal functions parseDate and splitNameContent @inline functions, not sure if it could be a problem ...

can you update the benchmark numbers as before and after. I think we can make more changes for performance :)

Instead of using None use null. This can cause null pointer exceptions but that should be fine because we will rename this method to unsafeDecodeResponseCookie
and make it package-private. And, we will have a separate method decodeResponseCookie which calls unsafeDecodeResponseCookie and wraps the output in an Option. And that should be a public method.

tusharmath · 2021-12-13T10:28:57Z

zio-http/src/main/scala/zhttp/http/Cookie.scala

+      val c       = if (i <= 0) headerValue.substring(prev) else headerValue.substring(prev, i)
+      val (n, ct) = splitNameContent(c)
+
+      (n.toLowerCase, ct) match {


unnecessary tuple allocation.

@tusharmath I have mode some changes to avoid string and tuple unnecessary allocations, inner loop is not the fanciest but I think that this way you get more performance ... what do you think ?

Can you commit the Benchmark code to the benchmarks project.

Sure, added CookieDecodeBenchark to zio-http-benchmarks (very naive at the moment)

tusharmath · 2021-12-16T10:17:13Z

zio-http/src/main/scala/zhttp/http/Cookie.scala

-      case _             => ("", None)
+  @inline
+  private def parseDate(v: String): Try[Instant] =
+    Try(Instant.parse(v))


Try will allocate additional memory. Instead, you could use a try { } catch {} block.
Incase of failure return a null

zio-http/src/main/scala/zhttp/http/Cookie.scala

tusharmath · 2021-12-16T10:18:50Z

zio-http-benchmarks/src/main/scala/zhttp.benchmarks/CookieDecodeBenchmark.scala

+@BenchmarkMode(Array(Mode.Throughput))
+@OutputTimeUnit(TimeUnit.SECONDS)
+class CookieDecodeBenchmark {
+  private val cookie = "SUID=123;httponly=true;expires=2007-12-03T10:15:30.00Z"


May be generate a larger cookie. With a large payload and more key-value pairs.

tusharmath · 2021-12-16T10:19:59Z

zio-http/src/main/scala/zhttp/http/Cookie.scala

+          } else {
+            name = headerValue.substring(0, next)
+          }
+        } else if (headerValue.regionMatches(true, curr, "expires=", 0, 8)) {


The string literals will also allocate memory every time the method is called. You could move it into a value so that it's allocated only once.

tusharmath · 2021-12-16T10:20:34Z

zio-http/src/main/scala/zhttp/http/Cookie.scala

+        } else if (headerValue.regionMatches(true, curr, "samesite=", 0, 9)) {
+          val v = headerValue.substring(curr + 9, next).toLowerCase
+          v match {
+            case "lax"    => sameSite = SameSite.Lax


I am not really sure but "lax" AFAIK will also allocate additional memory.

jgoday · 2021-12-16T21:24:18Z

@tusharmath
Updated with your latest suggestions:

String literals
Update benchmark with a larger payload cookie

OLD CODE:
[info] CookieDecodeBenchmark.benchmarkApp  thrpt    3  387562,885 ± 52538,364  ops/s

NEW CODE:
[info] CookieDecodeBenchmark.benchmarkApp  thrpt    3  679569,604 ± 27421,427  ops/s

Avoid scala.util.Try in parseDate
Added unsafeDecodeResponseCookie: Cookie
Really like the idea of having a cookie with the index-based fields, specially to avoid splitNameContent allocations in 'decodeRequestCookie', but as you said, maybe it's better to do it in another PR ...

zio-http/src/main/scala/zhttp/http/Cookie.scala

zio-http-benchmarks/src/main/scala/zhttp.benchmarks/CookieDecodeBenchmark.scala

zio-http/src/main/scala/zhttp/http/Cookie.scala

Co-authored-by: Tushar Mathur <tusharmath@gmail.com>

tusharmath · 2021-12-23T15:02:48Z

zio-http/src/main/scala/zhttp/http/Cookie.scala

+            try { Instant.parse(headerValue.substring(curr + 8, next)) }
+            catch { case _: Throwable => null }
+        } else if (headerValue.regionMatches(true, curr, fieldMaxAge, 0, fieldMaxAge.length)) {
+          maxAge = Try(headerValue.substring(curr + 8, next).toLong).toOption


We can create a global try catch block for this method.

@tusharmath , a couple of questions here:

would it be enough to wrap with a try each attempt to decode a field?

What should we do with the exception, discard it completely?

We should have only 1 top level try catch block inside the safe implementation.

Incase of failure we should throw an exception in the unsafe implementation. The safe implementation captures it and then could do either of the two things —
a. Ignore the error and return a None.
b. Capture the error and return a Left[Throwable]

Ok, changed to catch the exception in decodeResponseCookie but keeping the Option[Cookie] signature

tusharmath · 2021-12-28T06:25:42Z

Really looking forward to this PR @jgoday :)

zio-http-benchmarks/src/main/scala/zhttp.benchmarks/CookieDecodeBenchmark.scala

tusharmath · 2022-01-03T14:00:40Z

@jgoday Thanks! this was pretty amazing :) We make it 1.7x faster. It might not seem much right now, but it all adds up when write a proper e2e application.

jgoday · 2022-01-04T08:26:57Z

Thank you @tusharmath for all your help and support.

jgoday requested review from amitksingh1490 and tusharmath as code owners November 26, 2021 20:47

jgoday mentioned this pull request Nov 26, 2021

Improve performance of cookie parser #571

Closed

zio#571: Reduce decodeResponseCookie allocations

1cd3d23

jgoday force-pushed the Fix/cookie-decoder-allocations branch from 821a10c to 1cd3d23 Compare November 26, 2021 22:08

tusharmath reviewed Nov 27, 2021

View reviewed changes

jgoday added 4 commits November 27, 2021 23:13

avoid some allocations

adeb576

use while instead of do while (scala 3 compatibility)

f316e18

do not use string.isBlank

874cb0f

use string.isEmpty

ba5f32b

tusharmath reviewed Dec 13, 2021

View reviewed changes

Cookie decodeResponseCookie: avoid allocations

680aeb6

amitksingh1490 changed the base branch from release/next to main December 14, 2021 09:40

Add decode cookie benchmark code

2ce5fc9

tusharmath reviewed Dec 16, 2021

View reviewed changes

zio-http/src/main/scala/zhttp/http/Cookie.scala Show resolved Hide resolved

tusharmath reviewed Dec 16, 2021

View reviewed changes

jgoday added 2 commits December 16, 2021 22:11

String literals and update CookieDecodeBenchmark

dee49ca

add unsafeDecodeResponseCookie

ea55f0a

tusharmath reviewed Dec 21, 2021

View reviewed changes

zio-http/src/main/scala/zhttp/http/Cookie.scala Show resolved Hide resolved

tusharmath reviewed Dec 21, 2021

View reviewed changes

zio-http-benchmarks/src/main/scala/zhttp.benchmarks/CookieDecodeBenchmark.scala Show resolved Hide resolved

tusharmath reviewed Dec 21, 2021

View reviewed changes

zio-http/src/main/scala/zhttp/http/Cookie.scala Outdated Show resolved Hide resolved

tusharmath reviewed Dec 21, 2021

View reviewed changes

zio-http/src/main/scala/zhttp/http/Cookie.scala Outdated Show resolved Hide resolved

tusharmath added the performance label Dec 21, 2021

tusharmath changed the title ~~#571: Reduce decodeResponseCookie allocations~~ Reduce decodeResponseCookie allocations Dec 21, 2021

Update zio-http/src/main/scala/zhttp/http/Cookie.scala

68620fe

Co-authored-by: Tushar Mathur <tusharmath@gmail.com>

jgoday and others added 2 commits December 22, 2021 18:31

Update zio-http/src/main/scala/zhttp/http/Cookie.scala

ee30291

Co-authored-by: Tushar Mathur <tusharmath@gmail.com>

Update cookie decode benchmark

46d30dd

tusharmath approved these changes Dec 23, 2021

View reviewed changes

tusharmath reviewed Dec 23, 2021

View reviewed changes

do not catch exceptions in unsafeDecodeResponseCookie

9e7a7e6

tusharmath reviewed Dec 29, 2021

View reviewed changes

zio-http-benchmarks/src/main/scala/zhttp.benchmarks/CookieDecodeBenchmark.scala Outdated Show resolved Hide resolved

CookieDecodeBenchmark agains unsafeDecodeResponseCookie

58ee1bf

tusharmath merged commit 03881d9 into zio:main Jan 3, 2022

tusharmath changed the title ~~Reduce decodeResponseCookie allocations~~ Performance: Cookie decoder improvements Jan 3, 2022

jgoday mentioned this pull request Jan 30, 2022

Indexed fields cookie changes #924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance: Cookie decoder improvements #576

Performance: Cookie decoder improvements #576

jgoday commented Nov 26, 2021

codecov-commenter commented Nov 26, 2021 •

edited

tusharmath Nov 27, 2021 •

edited

jgoday Nov 27, 2021

tusharmath Dec 16, 2021

tusharmath Dec 16, 2021

tusharmath Dec 13, 2021

jgoday Dec 13, 2021

tusharmath Dec 15, 2021

jgoday Dec 15, 2021

tusharmath Dec 16, 2021

tusharmath Dec 16, 2021

tusharmath Dec 16, 2021

tusharmath Dec 16, 2021

jgoday commented Dec 16, 2021

tusharmath Dec 23, 2021

jgoday Dec 27, 2021

tusharmath Dec 28, 2021 •

edited

jgoday Dec 28, 2021

tusharmath commented Dec 28, 2021

tusharmath commented Jan 3, 2022 •

edited

jgoday commented Jan 4, 2022

Performance: Cookie decoder improvements #576

Performance: Cookie decoder improvements #576

Conversation

jgoday commented Nov 26, 2021

codecov-commenter commented Nov 26, 2021 • edited

Codecov Report

tusharmath Nov 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgoday commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tusharmath Dec 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tusharmath commented Dec 28, 2021

tusharmath commented Jan 3, 2022 • edited

jgoday commented Jan 4, 2022

codecov-commenter commented Nov 26, 2021 •

edited

tusharmath Nov 27, 2021 •

edited

tusharmath Dec 28, 2021 •

edited

tusharmath commented Jan 3, 2022 •

edited