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

refactor Jackson code to hardcode buffer-recycler #999

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pjfanning
Copy link
Contributor

@pjfanning pjfanning commented Mar 16, 2024

Pull Request Checklist

  • Have you read through the contributor guidelines?
  • Have you squashed your commits?
  • Have you added copyright headers to new files?
  • Have you updated the documentation?
  • Have you added tests for any changed functionality?

Fixes

partial fix for #998

Purpose

What does this PR do?

Background Context

Why did you take this approach?

JsonFactory should be used to create ObjectMappers - not the other way around.

ThreadLocal buffer-recycler is the default up to and including Jackson 2.16. Default was changed in Jackson 2.17 but the way play-json uses Jackson is likely to suffer performance wise with the new default.

You might want to make the buffer-recycler configurable like in apache/pekko-http#519

Upgrading to Jackson 2.15 and above also brings in Jackson's StreamReadConstraints and StreamWriteConstraints.

play-json has its own number parsing checks so I disabled the Jackson check. There are other checks in StreamReadConstraints and StreamWriteConstraints and play-json needs to decide if you want to make them configurable or if you want to disable the Jackson checks.

References

Are there any relevant issues / PRs / mailing lists discussions?

@pjfanning pjfanning force-pushed the patch-1 branch 3 times, most recently from 029724d to b3f7034 Compare March 16, 2024 16:10
refactor Jackson code to hardcode buffer-recycler

update jackson

scalafmt

Update JacksonJson.scala

compile issue
@plokhotnyuk
Copy link

I've used proposed changes to fix performance issues of play-json with latest jackson-core in this commit.

Here is a screenshot of top-10 list of improved benchmarks when running with GraalVM JDK 22 on Intel® Core™ i9-13900K CPU having DDR5-4800 RAM using 16 threads (only performance core were enabled):

image

Here are JMH result comparison pages for that and other JVMs using 16 threads on the same environment:

.builder()
.asInstanceOf[JsonFactoryBuilder]
.streamReadConstraints(streamReadConstraints)
.recyclerPool(JsonRecyclerPools.threadLocalPool())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jackson 2.17.0 used a very inefficient recycler pool that behaved even worse under heavy load - called 'Lock Free'. Jackson 2.17.1 and 2.17.2 (just released) goes back to ThreadLocalPool as the default. Still maybe a good idea to hardcode its use though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

2 participants