Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK 21 benchmarks #164

Closed
lowecg opened this issue Jan 10, 2024 · 8 comments
Closed

JDK 21 benchmarks #164

lowecg opened this issue Jan 10, 2024 · 8 comments
Assignees

Comments

@lowecg
Copy link

lowecg commented Jan 10, 2024

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
Copy link
Member

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
Copy link
Author

lowecg commented Jan 11, 2024

Apple MBP M1 Max, commit 79f0212

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
Copy link
Member

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
Copy link
Author

lowecg commented Jan 11, 2024

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
Copy link
Member

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
Copy link
Author

lowecg commented Jan 11, 2024

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
Copy link
Member

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 added a commit that referenced this issue Jan 15, 2024
ptaoussanis added a commit that referenced this issue Jan 15, 2024
@ptaoussanis
Copy link
Member

Benchmarks have been updated to use JDK 21 👍

ptaoussanis added a commit that referenced this issue Jan 15, 2024
ptaoussanis added a commit that referenced this issue Jan 15, 2024
ptaoussanis added a commit that referenced this issue Jan 15, 2024
ptaoussanis added a commit that referenced this issue Jan 15, 2024
ptaoussanis added a commit that referenced this issue Jan 15, 2024
ptaoussanis added a commit that referenced this issue Jan 15, 2024
ptaoussanis added a commit that referenced this issue Jan 16, 2024
ptaoussanis added a commit that referenced this issue Jan 16, 2024
ptaoussanis added a commit that referenced this issue Jan 16, 2024
ptaoussanis added a commit that referenced this issue Jan 16, 2024
ptaoussanis added a commit that referenced this issue Jan 16, 2024
ptaoussanis added a commit that referenced this issue Jan 17, 2024
ptaoussanis added a commit that referenced this issue Jan 17, 2024
ptaoussanis added a commit that referenced this issue Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants