taoensso / nippy

The fastest serialization library for Clojure
https://www.taoensso.com/nippy
Eclipse Public License 1.0
1.04k stars 60 forks source link

JDK 21 benchmarks #164

Closed lowecg closed 5 months ago

lowecg commented 5 months ago

You're probably aware of this already, but JDK 21 has some nice improvements around DataOutputStream. Unless I'm mistaken, Nippy uses DOS?

Have you run the benchmarks against JDK 21?

ptaoussanis commented 5 months ago

Hi there! Nippy does indeed use DOS. And thanks for pinging about this, I actually wasn't aware of specific DOS improvements.

I can't recall off-hand what JVM I used for the last benchmarks - but I'll update them for the next pre-release using JDK 21 👍 Will also document the JDK version used for benchmarks.

lowecg commented 5 months ago

Apple MBP M1 Max, commit 79f0212431d3e387ff39a48a6549318ae11167f2

Corretto 17.0.9

{:reader {:round 20391, :freeze 4299, :thaw 16092, :size 39454},
 :fressian {:round 3890, :freeze 1911, :thaw 1979, :size 22887},
 :nippy/lzma2 {:round 23816, :freeze 13722, :thaw 10094, :size 11476},
 :nippy/encrypted {:round 6817, :freeze 3462, :thaw 3355, :size 16745},
 :nippy/default {:round 1832, :freeze 967, :thaw 865, :size 16717},
 :nippy/fast {:round 1688, :freeze 857, :thaw 831, :size 28495}}

Corretto 21.0.1

{:reader {:round 21283, :freeze 4232, :thaw 17051, :size 39434},
 :fressian {:round 4013, :freeze 2035, :thaw 1978, :size 22890},
 :nippy/lzma2 {:round 24906, :freeze 14712, :thaw 10194, :size 11464},
 :nippy/encrypted {:round 1988, :freeze 1068, :thaw 920, :size 16747},
 :nippy/default {:round 1872, :freeze 1009, :thaw 863, :size 16719},
 :nippy/fast {:round 1734, :freeze 904, :thaw 830, :size 28497}}

I was expecting a different result for the default and fast cases (hoping to see a change in the other direction), but look at the encrypted score! I ran the benchmark a few times to be sure.

It's not just nippy times; reader and fressian have also increased, so something else may be at play here.

The size has changed between the versions in all tests, so further scrutiny is warranted. That said, my checksum tests for Nippy output didn't flag anything for my use case.

ptaoussanis commented 5 months ago

Thanks for checking @lowecg!

Re: no improvement in default/fast cases from JVM 17 to 21

Sorted explanations that come to mind:

  1. The JVM optimizations aren't relevant in Nippy's case (e.g. DOS already wasn't a bottleneck, etc.).
  2. The JVM optimizations are somehow being thwarted (e.g. might require a tweak to usage to see benefits, etc.).

Re: data size changing

That's normal behaviour since the current stress data includes random values. The data will be fixed for a given set of bench runs, but may vary between runs (e.g. when you're starting up a different JVM).

There's no specific motivation to include these random values so they could be removed, though it's also currently unclear to me what the benefit would be (besides maybe reducing possible confusion as in this case).

I'd be up for a simple PR that made the stress data deterministic.

Re: encryption speed improvements

The improvement here is large enough to be suspicious.

Sorted explanations:

  1. Encryption is actually somehow not working for your JVM 21 test (are the encryption unit tests all passing?).
  2. Major relevant improvements were made to JVM 21's cryptographic performance.
lowecg commented 5 months ago
image

Only the serialisation test is failing:

Expected

#{"add.1"
  "add.2"
  "base.1"
  "base.2"}

Actual

#{"[B"
  "[C"
  "[D"
  "[F"
  "[I"
  "[J"
  "[S"
  "[Z"
  "clojure.lang.ArityException"
  "clojure.lang.ExceptionInfo"
  "java.lang.ArithmeticException"
  "java.lang.ClassCastException"
  "java.lang.Exception"
  "java.lang.IllegalArgumentException"
  "java.lang.IndexOutOfBoundsException"
  "java.lang.NullPointerException"
  "java.lang.RuntimeException"
  "java.lang.Throwable"
  "java.net.URI"
  "java.time.Clock"
  "java.time.DateTimeException"
  "java.time.LocalDate"
  "java.time.LocalDateTime"
  "java.time.LocalTime"
  "java.time.MonthDay"
  "java.time.OffsetDateTime"
  "java.time.OffsetTime"
  "java.time.Year"
  "java.time.YearMonth"
  "java.time.ZoneId"
  "java.time.ZoneOffset"
  "java.time.ZonedDateTime"
  "org.joda.time.DateTime"}
ptaoussanis commented 5 months ago

Only the serialisation test is failing

Those will fail if you're running the unit tests in a REPL, etc. because they depend on specific JVM options being in place.

They should pass via lein test, etc.

lowecg commented 5 months ago

Ah - I was running the benchmarks from a REPL and the tests from within Cursive.

Using lein test, I see there is an issue!

JDK 17

% lein test

lein test taoensso.nippy-tests
Clojure version: {:major 1, :minor 11, :incremental 1, :qualifier nil}

Starting benchmarks
- Benching Nippy/encrypted...
- Benching Nippy/default...
- Benching Nippy/fast...
- Benchmarks complete! (Time for cake?)

Benchmark results:
:nippy/encrypted   {:round 6911, :freeze 3511, :thaw 3400, :size 16745}
:nippy/default   {:round 1795, :freeze 966, :thaw 829, :size 16717}
:nippy/fast   {:round 1656, :freeze 864, :thaw 792, :size 28495}

Ran 14 tests containing 568 assertions.
0 failures, 0 errors.

JDK 21 crashes spectacularly hs_err_pid73409.log !

% lein test

lein test taoensso.nippy-tests
Clojure version: {:major 1, :minor 11, :incremental 1, :qualifier nil}
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00000001375772b8, pid=73409, tid=8451
#
# JRE version: OpenJDK Runtime Environment Corretto-21.0.1.12.1 (21.0.1+12) (build 21.0.1+12-LTS)
# Java VM: OpenJDK 64-Bit Server VM Corretto-21.0.1.12.1 (21.0.1+12-LTS, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-aarch64)
# Problematic frame:
# v  ~StubRoutines::forward_copy_longs 0x00000001375772b8
#
# No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# An error report file with more information is saved as:
# /Users/lowecg/source/external/nippy/hs_err_pid73409.log
[10.109s][warning][os] Loading hsdis library failed
#
# If you would like to submit a bug report, please visit:
#   https://github.com/corretto/corretto-21/issues/
#
ptaoussanis commented 5 months ago

JDK 21 crashes spectacularly

That's likely intermittent and unrelated to the JDK version.

v3.4.0-beta1 uses a different compression library that seems to be susceptible to core dumps when attempting to decrypt invalid (e.g. random) data.

There's a test for this here.

Haven't decided yet what to do about the problem. ~Sorted options include:

  1. Upstream fix in the compression lib.
  2. Other Nippy-level workaround to prevent possibility of these crashes.
  3. Switch back to the old compression lib.

Was planning to investigate further and potentially report upstream when prepping the next v3.4 pre-release.

ptaoussanis commented 5 months ago

Benchmarks have been updated to use JDK 21 👍