Add compression to the metadata code snapshot #11470

4e6 · 2024-11-01T17:26:39Z

Pull Request Description

close #11420

Changelog:

update: add zlib compression to the snapshot metadata field
add: implement nodejs zlib for polyglot ydoc-server
add: implement nodejs Buffer for polyglot ydoc-server

Important Notes

Checklist

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

The documentation has been updated, if necessary.
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.

4e6 · 2024-11-01T17:27:49Z

app/ydoc-server-polyglot/build.mjs

+  'node:zlib': {
+    varName: 'zlib',
+    type: 'cjs',
+  }


make zlib a global symbol in the bundle instead of require('node:zlib')

4e6 · 2024-11-01T17:33:21Z

app/ydoc-server/src/languageServerSession.ts

@@ -505,6 +513,7 @@ class ModulePersistence extends ObservableV2<{ removed: () => void }> {
    const newSnapshot = newCode && {
      snapshot: ModulePersistence.encodeCodeSnapshot(newCode),
    }
+    if (newMetadata) newMetadata.snapshot = this.syncedMeta.ide.snapshot


The #11435 changes the newMetadata argument to store the ide object:

- newMetadata: fileFormat.IdeMetadata['node'] | undefined, + newMetadata: fileFormat.IdeMetadata | undefined,

This change https://github.com/enso-org/enso/pull/11435/files#diff-f621895c66ac2e5e220af1fb6ce9cd3694e5f85897f471300ad63a7c9f5bee9aL511-L513 builds the new metadata object without the snapshot field if there are no code changes. I.e. it makes the snapshot field disappear from the file on metadata changes (moving the node, for example)

4e6 · 2024-11-01T17:40:22Z

app/ydoc-server/src/fileFormat.ts

    widget: z.optional(z.record(z.string().uuid(), z.record(z.string(), z.unknown()))),
+    snapshot: z.string().optional(),


Make the snapshot the last field. The ydoc diff algorithm places the snapshot at the end of the metadata. This prevents unnecessary edits.

Oh, that's subtle--it could use a comment.

kazcw · 2024-11-01T17:46:28Z

app/ydoc-server/src/languageServerSession.ts

+  private static encodeCodeSnapshot(code: string): string | undefined {
+    try {
+      return zlib.deflateSync(Buffer.from(code, 'utf8')).toString('base64')
+    } catch {
+      return
+    }
  }

-  private static decodeCodeSnapshot(snapshot: string): string {
-    return Base64.decode(snapshot)
+  private static decodeCodeSnapshot(snapshot: string): string | undefined {
+    try {
+      return zlib.inflateSync(Buffer.from(snapshot, 'base64')).toString('utf8')
+    } catch {
+      return
+    }


I think any errors here are worth logging

JaroslavTulach

I'd like to use this PR as an opportunity to restate that #11390 seems to miss two essential changes:

versioning
treat meta-data as a comment

The change of the snapshot field content format done in this PR shows how essential versioning is. It should become part of the format asap, imo.

JaroslavTulach · 2024-11-04T04:46:07Z

lib/java/ydoc-server/src/test/java/org/enso/ydoc/polyfill/web/ZlibTest.java

+
+    context.getBindings("js").putMember("TEXT", TEXT);
+
+    var result = CompletableFuture.supplyAsync(() -> context.eval("js", code), executor).get();


Rather than playing with global variables you should define a JavaScript function:

var code = "(function(text) { return Buffer.from(text).toString(); })";

and then invoke it:

var fn = context.eval("js", code); return fn.execute(TEXT);

GraalJS sets up tests this way https://github.com/search?q=repo%3Aoracle%2Fgraaljs%20putMember&type=code. I think it's cleaner than writing a function.

hubertp · 2024-11-04T11:32:40Z

lib/java/ydoc-server/src/main/java/org/enso/ydoc/polyfill/web/Zlib.java

+            Charset charset;
+            try {
+              charset = Charset.forName(encoding);
+            } catch (IllegalArgumentException ignored) {


Sounds like we should log this at least.

Made it compatible with the nodejs by returning an error.

hubertp · 2024-11-04T11:32:56Z

lib/java/ydoc-server/src/main/java/org/enso/ydoc/polyfill/web/Zlib.java

+            Charset charset;
+            try {
+              charset = Charset.forName(encoding);
+            } catch (IllegalArgumentException ignored) {


Same as above

hubertp · 2024-11-04T11:34:24Z

lib/java/ydoc-server/src/main/java/org/enso/ydoc/polyfill/web/Zlib.java

+
+    return switch (command) {
+      case BUFFER_FROM -> {
+        final var text = arguments[1].asString();


It looks like we can easily get IndexOutOfBounds exceptions when not checking the size of the arguments?

This method is called from zlib.js and IndexOutOfBounds exception would indicate an error in the implementation. Plus these paths are checked in tests.

4e6 · 2024-11-04T15:26:01Z

Metadata versioning will be implemented in #11479

…shot

4e6 added 3 commits November 1, 2024 16:15

feat: zlib snapshot compression

fa9d91d

feat: zlib polyfill

2730817

feat: deflate output stream

ac9edad

4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 1, 2024

4e6 self-assigned this Nov 1, 2024

4e6 requested review from Frizi, farmaazon, vitvakatu, kazcw, AdRiley, JaroslavTulach, hubertp and Akirathan as code owners November 1, 2024 17:26

4e6 commented Nov 1, 2024

View reviewed changes

kazcw approved these changes Nov 1, 2024

View reviewed changes

misc: prettier

86cb95a

JaroslavTulach reviewed Nov 4, 2024

View reviewed changes

hubertp approved these changes Nov 4, 2024

View reviewed changes

4e6 added 2 commits November 4, 2024 15:24

update: encoding implementation

a76c992

update: handle exceptions

65cd297

4e6 added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Nov 4, 2024

4e6 added 2 commits November 4, 2024 21:41

Merge branch 'develop' into wip/db/11420-add-compression-to-code-snap…

f7ea455

…shot

Merge branch 'develop' into wip/db/11420-add-compression-to-code-snap…

244580e

…shot

4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Nov 5, 2024

mergify bot merged commit 47943a2 into develop Nov 5, 2024
41 of 42 checks passed

mergify bot deleted the wip/db/11420-add-compression-to-code-snapshot branch November 5, 2024 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compression to the metadata code snapshot #11470

Add compression to the metadata code snapshot #11470

4e6 commented Nov 1, 2024

4e6 Nov 1, 2024

4e6 Nov 1, 2024

4e6 Nov 1, 2024

kazcw Nov 1, 2024

kazcw Nov 1, 2024

JaroslavTulach left a comment

JaroslavTulach Nov 4, 2024

4e6 Nov 4, 2024

hubertp Nov 4, 2024

4e6 Nov 4, 2024

hubertp Nov 4, 2024

hubertp Nov 4, 2024

4e6 Nov 4, 2024

4e6 commented Nov 4, 2024

		widget: z.optional(z.record(z.string().uuid(), z.record(z.string(), z.unknown()))),
		snapshot: z.string().optional(),


		context.getBindings("js").putMember("TEXT", TEXT);

		var result = CompletableFuture.supplyAsync(() -> context.eval("js", code), executor).get();

Add compression to the metadata code snapshot #11470

Add compression to the metadata code snapshot #11470

Conversation

4e6 commented Nov 1, 2024

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4e6 commented Nov 4, 2024