Simplify and optimize prettyPrint and stringify methods

[gaeljw]

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?

Purpose

This PR simplifies the code of JacksonJson (JVM) that is used on the path of the Json.stringify, Json.asciiStringify, Json.prettyPrint and Json.prettyPrintToStream methods.

The simplification consists of:

• removing unnecessary creation of a JsonGenerator at each call (mapper.getFactory.createGenerator)
  • this also reduces duplication, for instance the prettyPrint only set the DefaultPrettyPrinter once rather than doing it once on the ObjectWriter and once on the JsonGenerator (to be fair, I don't know if both were useful or one would have been enough anyways)
• removing unnecessary creation of a StringWriter: Jackson handles it itself.
  • As per Jackson's documentation:

    writeValueAsString: Functionally equivalent to calling writeValue(Writer, Object) with StringWriter and constructing String, but more efficient.

On top of code simplification, this also brings performance improvements.

A minimal JMH benchmark (not included in the PR but I could if you think it's worth it) gives the following results on my laptop:

[info] Benchmark                                     Mode  Cnt     Score     Error   Units

[info] PrettyPrintBench.asciiStringify_before       thrpt   10   703,628 ±  18,430  ops/ms
[info] PrettyPrintBench.asciiStringify              thrpt   10  1303,345 ±  20,933  ops/ms

[info] PrettyPrintBench.stringify_before            thrpt   10   708,859 ±  14,447  ops/ms
[info] PrettyPrintBench.stringify                   thrpt   10  1316,503 ±  43,704  ops/ms

[info] PrettyPrintBench.prettyPrintToStream_before  thrpt   10   299,436 ± 134,214  ops/ms
[info] PrettyPrintBench.prettyPrintToStream         thrpt   10   952,180 ±  91,934  ops/ms

[info] PrettyPrintBench.prettyPrint_before          thrpt   10   571,337 ±  13,095  ops/ms
[info] PrettyPrintBench.prettyPrint                 thrpt   10   992,202 ±  47,259  ops/ms

While this is a very basic benchmark on a single case (a single JsValue), I think it's safe to assume that the code change improves performance.

Benchmark code

package play.api.libs.json

import org.openjdk.jmh.annotations._
import org.openjdk.jmh.util.NullOutputStream
import play.api.libs.json.jackson.JacksonJson

import java.io.OutputStream
import java.util.concurrent.TimeUnit

/**
 * ==Quick Run==
 * benchmarks / Jmh / run .*PrettyPrintBench
 *
 */
@Warmup(iterations = 20, time = 1, timeUnit = TimeUnit.SECONDS)
@Measurement(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
@State(Scope.Benchmark)
@BenchmarkMode(Array(Mode.Throughput))
@OutputTimeUnit(TimeUnit.MILLISECONDS)
@Fork(
  jvmArgsAppend =
    Array("-Xmx350m", "-XX:+HeapDumpOnOutOfMemoryError", "-XX:-BackgroundCompilation", "-XX:-TieredCompilation"),
  value = 1
)
class PrettyPrintBench {

  private var jsObject: JsObject = _
  private var outputStream: OutputStream = _

  @Setup def setup(): Unit = {
    jsObject = Json.obj(
      "key1" -> "toto",
      "key2" -> Json.obj("key21" -> "tata", "key22" -> 123),
      "key3" -> Json.arr(1, "tutu")
    )
    outputStream = new NullOutputStream()
  }
  
  @TearDown
  def clean() = {
    outputStream.close()
  }

  @Benchmark
  def prettyPrint = {
    JacksonJson.get.prettyPrint(jsObject)
    ()
  }
  
  @Benchmark
  def stringify = {
    JacksonJson.get.generateFromJsValue(jsObject, false)
    ()
  }
  
  @Benchmark
  def asciiStringify = {
    JacksonJson.get.generateFromJsValue(jsObject, true)
    ()
  }

  @Benchmark
  def prettyPrintToStream = {
    JacksonJson.get.prettyPrintToStream(jsObject, outputStream)
    ()
  }

}

Background Context

I noticed this when working on the upgrade to Jackson 3 (#1268) and I think it's worth a separate PR before any upgrade to Jackson 3.

Additional notes

• The 1st commit may seem unrelated, I can open a dedicated PR for it if you prefer (or squash it with the 2nd). In practice it's partially related because at some point (again when working on the Jackson 3 upgrade), I was able to get tests working when comparing strings but the underlying object (Jackson JsonNode) was not of the right type.

• Having 2 mappers (one without and one with the ESCAPE_NON_ASCII feature) goes in the direction of Jackson 3 where these objects are immutable

[pjfanning]

lgtm

[mkurz]

While reviewing the refactor against Jackson’s behavior, I found two compatibility regressions and pushed two focused follow-up commits:

• e39feaa configures non-ASCII escaping on a per-call ObjectWriter instead of copying the configured ObjectMapper. This supports custom mapper subclasses, keeps later mapper configuration visible, restores the zero-argument mapper() JVM method, and preserves setObjectMapper(null) reset behavior.
• 568d031 disables Jackson’s AUTO_CLOSE_TARGET for prettyPrintToStream. The new writeValue(OutputStream, ...) overload otherwise closes the caller-owned stream, unlike the previous generator-based implementation.

[mkurz]

@gaeljw what do you think?

[mkurz]

@gaeljw I will merge that as is, we can improve things later if you have findings.

[gaeljw]

Thanks @mkurz , LGTM.

IIRC I went with the "copy" because we'll have to with Jackson 3 as ObjectMapper are immutable. But this is good enough for now :)