Replace Jackson serde #10035

hubertp · 2024-05-22T12:26:58Z

Pull Request Description

JSON serialization setup between Language Server and Runtime is a major contributor to startup time. This PR experiments with an alternative implementation that remedies the problem.
The new serializer uses jsoniter-scala which by some accounts claims to be really fast. In our case, more importantly, we pay negligible cost of startup setup compared to Jackson which was horribly slow.

Important Notes

Before:

After:

Yes. About 0.8sec.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

1. Avoiding loading cats. It will we loaded later anyway but `catchNonFatal` is not worth it anyway 2. Avoid copying when reading all bytes from a file to compute digest of all files

hubertp · 2024-05-22T13:17:12Z

Nit: what I really like about this library is that, when it works, the context of the parsing error is pretty visible:

com.github.plokhotnyuk.jsoniter_scala.core.JsonReaderException: expected '{' or null, offset: 0x0000018a, buf:
+----------+-------------------------------------------------+------------------+
|          |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f | 0123456789abcdef |
+----------+-------------------------------------------------+------------------+
| 00000160 | 72 6f 75 70 73 22 3a 5b 7b 22 67 72 6f 75 70 22 | roups":[{"group" |
| 00000170 | 3a 7b 22 6e 61 6d 65 22 3a 22 49 6e 70 75 74 22 | :{"name":"Input" |
| 00000180 | 7d 2c 22 63 6f 6c 6f 72 22 3a 22 6f 6b 6c 63 68 | },"color":"oklch |
| 00000190 | 28 30 2e 35 34 35 20 30 2e 31 34 20 31 37 30 29 | (0.545 0.14 170) |
| 000001a0 | 22 2c 22 65 78 70 6f 72 74 73 22 3a 5b 5d 7d 2c | ","exports":[]}, |
+----------+-------------------------------------------------+------------------+

Turned out that one needs to separate definition site of the serializer from usage so that macros can do the magic properly.

JaroslavTulach

Please don't change dependencies of engine-common.

build.sbt

When having nested option string values, decoders and encoders don't seem to match under certain conditions. Added an option to skip the values instead, which appears to remedy the problems. Moving projects around to properly setup macros.

Akirathan

Nice startup speedup. I am just not sure what is the purpose of the new polyglot-api-macros project. It seems to me that the single source in that project can be merged into polyglot-api-serde project.

Akirathan · 2024-06-11T07:56:09Z

engine/runtime/src/main/java/org/enso/interpreter/caches/CacheUtils.java

@@ -82,7 +85,14 @@ static final String computeDigestOfLibrarySources(List<SourceFile<TruffleFile>>
    try {
      var digest = messageDigest();
      for (var source : pkgSources) {
-        digest.update(source.file().readAllBytes());


I assume this change is not related to the json serialization? I am not sure whether this computeDigestOfLibrarySources is a useful method at all. It is used only by ImportExportCache. And when ImportExportCache [deserializes]

enso/engine/runtime/src/main/java/org/enso/interpreter/caches/ImportExportCache.java

Line 78 in 2ea2a57

return new CachedBindings(libraryName, bindings, Optional.empty());

), it does not even read the sources from the cache. Maybe we should get rid of this method and compute digest in some other, more performant way in ImportExportCache?

It's not related no. I just found out that this was taking unnecessary amount of time while profiling.

engine/polyglot-api-macros/src/main/scala/org/enso/polyglot/macros/SerdeConfig.scala

Akirathan · 2024-06-11T08:01:28Z

build.sbt

@@ -1478,6 +1478,32 @@ lazy val `polyglot-api` = project
  .dependsOn(`logging-utils`)
  .dependsOn(testkit % Test)

+lazy val `polyglot-api-macros` = project


What is the purpose of polyglot-api-macros project? Do I see correctly that it contains only single source SerdeConfig.scala? Shouldn't it be in polyglot-api-serde?

Yes, this is necessary due to jsoniter-scala's usage of Scala macros. Macros have to be compiled separately from the usage site. Alternatively we could put everything into one module but then we would have to define some (simple) macros. I'd prefer to avoid that due to maintenance cost on our side.

JaroslavTulach

Are we removing Jackson from our runtime.jar and runner.jar modules altogether?

build.sbt

JaroslavTulach · 2024-06-11T08:27:34Z

engine/polyglot-api/src/main/scala/org/enso/polyglot/ExportedSymbol.scala

@@ -1,28 +1,7 @@
 package org.enso.polyglot

-import com.fasterxml.jackson.annotation.{JsonSubTypes, JsonTypeInfo}


Removing Jackson import is good. Do we also remove the Jackson dependency?

I did, see build.sbt.

engine/polyglot-api/src/main/scala/org/enso/polyglot/runtime/Runtime.scala

...ration-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerEmulator.scala

...ne/com.fasterxml.jackson.dataformat.jackson-dataformat-cbor-2.15.2/files-add/CREDITS-2.x.txt

hubertp added 2 commits May 19, 2024 22:26

Minor perf nits discovered while profiling

e3a83e0

1. Avoiding loading cats. It will we loaded later anyway but `catchNonFatal` is not worth it anyway 2. Avoid copying when reading all bytes from a file to compute digest of all files

Experimenting with jsoniter-scala

e784da3

hubertp mentioned this pull request May 22, 2024

Non-deterministic codegen for Option deserialization plokhotnyuk/jsoniter-scala#1146

Closed

hubertp added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label May 23, 2024

hubertp added 3 commits June 6, 2024 09:41

Merge branch 'develop' into wip/hubert/9789-perf-nits

6663951

Fix jsoniter integration

148299a

Turned out that one needs to separate definition site of the serializer from usage so that macros can do the magic properly.

fmt

d7d3485

hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 6, 2024

hubertp added 3 commits June 6, 2024 17:30

package renaming

f1246fd

fix tests

3369a0b

one more rename

926c77e

JaroslavTulach requested changes Jun 7, 2024

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

Workaround nested option values jsoniter issue

fdb11be

When having nested option string values, decoders and encoders don't seem to match under certain conditions. Added an option to skip the values instead, which appears to remedy the problems. Moving projects around to properly setup macros.

hubertp requested a review from JaroslavTulach June 7, 2024 15:23

hubertp marked this pull request as ready for review June 7, 2024 15:27

hubertp requested review from 4e6 and Akirathan as code owners June 7, 2024 15:27

nits

40e84ac

enso-bot bot mentioned this pull request Jun 7, 2024

Avoid global compilation locks with visualizations to speedup feedback #9736

Closed

hubertp added 2 commits June 10, 2024 13:48

Drop jackson annotations

6f27c52

legal review

61c6c3b

hubertp requested review from radeusgd, jdunkerley, GregoryTravis, AdRiley and marthasharkey as code owners June 10, 2024 14:00

hubertp added 2 commits June 10, 2024 16:15

Merge branch 'develop' into wip/hubert/9789-perf-nits

0760638

missing licensing

3125387

hubertp changed the title ~~Experiment with replacing Jackson serde~~ Replace Jackson serde Jun 10, 2024

Akirathan approved these changes Jun 11, 2024

View reviewed changes

JaroslavTulach reviewed Jun 11, 2024

View reviewed changes

hubertp added 2 commits June 11, 2024 10:43

fmt changes

6733312

merge polyglot-api-serde to polyglot-api

35ff3c9

hubertp requested a review from JaroslavTulach June 11, 2024 10:01

JaroslavTulach approved these changes Jun 11, 2024

View reviewed changes

fmt

da7252b

radeusgd approved these changes Jun 11, 2024

View reviewed changes

hubertp added 3 commits June 11, 2024 13:47

licensing

e46b4c5

ditch jackson, only used in tests

2779406

missed licensing issues in PM

6ed4b46

hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Jun 11, 2024

mergify bot merged commit 4da5e61 into develop Jun 11, 2024
37 checks passed

mergify bot deleted the wip/hubert/9789-perf-nits branch June 11, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Jackson serde #10035

Replace Jackson serde #10035

hubertp commented May 22, 2024 •

edited

hubertp commented May 22, 2024

JaroslavTulach left a comment

Akirathan left a comment

Akirathan Jun 11, 2024

hubertp Jun 11, 2024

Akirathan Jun 11, 2024

hubertp Jun 11, 2024

JaroslavTulach left a comment

JaroslavTulach Jun 11, 2024

hubertp Jun 11, 2024

		@@ -1,28 +1,7 @@
		package org.enso.polyglot

		import com.fasterxml.jackson.annotation.{JsonSubTypes, JsonTypeInfo}

Replace Jackson serde #10035

Replace Jackson serde #10035

Conversation

hubertp commented May 22, 2024 • edited

Pull Request Description

Important Notes

Checklist

hubertp commented May 22, 2024

JaroslavTulach left a comment

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

Akirathan Jun 11, 2024

Choose a reason for hiding this comment

hubertp Jun 11, 2024

Choose a reason for hiding this comment

Akirathan Jun 11, 2024

Choose a reason for hiding this comment

hubertp Jun 11, 2024

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Jun 11, 2024

Choose a reason for hiding this comment

hubertp Jun 11, 2024

Choose a reason for hiding this comment

hubertp commented May 22, 2024 •

edited