Runtime: more efficient (space-wise) marshalling. by corwin-of-amber · Pull Request #814 · ocsigen/js_of_ocaml

corwin-of-amber · 2019-06-10T09:52:11Z

Modified caml_output_val to correctly reflects sharing.
As a result, it avoids duplication and saves a lot of space.

This has been thoroughly tested by compiling Coq .vo files, where it can reduce the file size by an order of magnitude, and produces roughly the same size as the native coqc.

Modified caml_output_val to correctly reflects sharing. As a result, it avoids duplication and saves a lot of space.

hhugo · 2019-06-10T12:54:55Z

Fixes #359

vouillon · 2019-06-10T13:07:45Z

What about using a WeakMap?

corwin-of-amber · 2019-06-10T13:13:45Z

What about using a WeakMap?

Or a regular Map for that matter... yes, that could be done and will improve the time complexity (saving the need for linear search in the internal object table). Question is if it's ok to rely on ES2015 specs in the runtime portion.

hhugo · 2019-06-10T13:40:48Z

Thanks a lot for this.

Would you mind writing some tests ? There are plenty of examples in https://github.com/ocsigen/js_of_ocaml/blob/master/compiler/tests/.

It would be nice to demonstrate that cyclic value can now be marshaled.

corwin-of-amber · 2019-06-10T17:01:46Z

Alas, I failed to build the latest version from source, due to
Error: Unbound value Ast_mapper.make_error_of_message

js_of_ocaml/ppx/ppx_js/lib_internal/ppx_js_internal.ml

Line 462 in 8022d1e

Ast_mapper.make_error_of_message

It seemed to work fine in Travis, so I looked at the configuration file and was not sure how to proceed, does the script (tools/travis.sh) even compile the source?

I am building on Mac with OCaml 4.07.1.

corwin-of-amber · 2019-06-10T17:08:18Z

Ok I ran opam install -y cohttp-lwt-unix menhir ppx_expect and now it builds except for the missing graphics package.

(I think it caused it to upgrade ocaml-migrate-parsetree, which for some reason, was not done by opam install -y --deps-only js_of_ocaml.)

hhugo · 2019-06-10T17:25:02Z

The ocaml-migrate-parsetree {>= "1.3.1"} constraint is not knonw to the opam-repository yet.
I would not expect opam to upgrade unless you pin js_of_ocaml.

About the missing graphics lib, I think building the compiler and its (new) tests should be enough (dune build @compiler/runtest)

corwin-of-amber · 2019-06-10T18:05:25Z

Added one test for shared data (DAG) and one for cycles (based on #359).

I noticed that for the other tests in channel.ml, you added OCaml counterparts. Do you want that for the two new ones as well?

hhugo · 2019-06-10T20:22:59Z

Tests look good as they are now. Maybe move them in their own file.

corwin-of-amber · 2019-06-10T20:29:57Z

Ok, suggestions on how to name it? I can't use compiler/tests/marshal.ml because it clashes with stdlib.

Test sharing and cyclic data.

hhugo · 2019-06-11T01:10:20Z

To follow on @vouillon remark. Could we you weakMap/Map with some fallback if not available ?

We should probably also check the flags in caml_output_value
https://github.com/ocsigen/js_of_ocaml/blob/master/runtime/io.js#L389 and
caml_output_value_to_{buffer,string,bytes} https://github.com/ocsigen/js_of_ocaml/blob/master/runtime/marshal.js#L445

corwin-of-amber · 2019-06-11T08:15:54Z

To follow on @vouillon remark. Could we you weakMap/Map with some fallback if not available ?

Good idea.

We should probably also check the flags in caml_output_value

Can easily support Marshal.No_sharing and Marshal.Compat32, I don't know about Marshal.Closures.
Are the flags the same for caml_output_value and for caml_output_val?

Where available; fallback to list is used for pre-2015 envs.

corwin-of-amber · 2019-06-11T12:36:18Z

+//Provides: MlObjectTable
+var MlObjectTable;
+if (typeof joo_global_object.WeakMap === 'undefined') {
+  MlObjectTable = function() {


In the current test setup this code is never tested because the condition is always false. 😞

I've tested the code manually.

Only Marshal.No_sharing makes sense in this context. Marshal.Closures cannot be supported currently. Marshal.Compat_32 is meaningless since all JavaScript integers are 32 bit.

corwin-of-amber · 2019-06-12T09:49:20Z

Ok, flags are in.

hhugo · 2019-06-13T00:16:40Z

+    function memo(v) {
+      var existing_offset = no_sharing ? undefined : intern_obj_table.recall(v);
+      if (existing_offset) { writer.write_shared(existing_offset); return existing_offset; }
+      else intern_obj_table.store(v);


do we need to call intern_obj_table.store when no_sharing is defined ?

We still need to update the object counter.
I can do writer.obj_counter++ instead, and remove the assignment writer.obj_counter = intern_obj_table.length at the end of the function.

obj_counter seems to only be used when sharing is enable.
https://github.com/ocaml/ocaml/blob/trunk/runtime/extern.c#L180

hhugo · 2019-06-15T04:55:19Z

I took the liberty to push to your branch directly (hope you don't mind).
Could you review my changes ?

corwin-of-amber · 2019-06-15T10:54:51Z

Looking great.
I noticed that memo(v) can in fact return a boolean. So I tweaked it further.

hhugo · 2019-06-15T14:14:40Z

Thanks a lot !

…riving_json, js_of_ocaml-tyxml, js_of_ocaml-ppx, js_of_ocaml-lwt, js_of_ocaml-compiler and js_of_ocaml-toplevel (3.5.0) CHANGES: ## Features/Changes * Compiler: Improve testing of the compiler (Ty Overby) * Compiler: Add several macros for making the runtime easier to maintain (ocsigen/js_of_ocaml#771) (Ty Overby) * Compiler: Allow to emit one javascript per compilation unit (ocsigen/js_of_ocaml#783) * Compiler: refactoring (ocsigen/js_of_ocaml#781, ocsigen/js_of_ocaml#782, ocsigen/js_of_ocaml#787, ocsigen/js_of_ocaml#795, ocsigen/js_of_ocaml#802) * Compiler: more source map location for the javascript runtime (ocsigen/js_of_ocaml#795) * Compiler: tune variable naming (ocsigen/js_of_ocaml#838) * Compiler: Work around num lib incompatibility * Compiler: escape '</' in strings (ocsigen/js_of_ocaml#899) * Compiler: speedup toplevel creation * Runtime: support sharing when marshaling (ocsigen/js_of_ocaml#814) * Runtime: add caml_obj_with_tag * Runtime: support marshaling custom block * Runtime: complete rewrite of bigarray * Runtime: complete num implementation * Runtime: add caml_ba_hash * Runtime: rewrite polymorphic compare * Ppx: switch ppx rewriter to the OCaml 4.08 ast * Misc: Improve CI speed * Misc: remove ppx_deriving dependency * Misc: remove cppo dependency * Misc: remove ppx_tools_versioned dependency in ppx_deriving_json * Misc: support for ocaml 4.09 * Misc: switch to ocamlformat.0.12 * Misc: many more tests * Misc: new jsoo_fs tool to embed files in a jsoo pseudo fs. * Lib: Use expect tests * Lib: Add support for 'addEventListener' with options (ocsigen/js_of_ocaml#807) * Lib: Change api of [Lwt_js_events.async] (ocsigen/js_of_ocaml#862) * Lib: Change api of responseText in xmlhttprequest (ocsigen/js_of_ocaml#863) * Lib: add resizeObserver bindings * Lib: Added support for custom events (ocsigen/js_of_ocaml#877) * Lib: Added support for focus events (ocsigen/js_of_ocaml#885) * Lib: Added `passive` option support for `Lwt_js_events` module * Lib: Added bindings for pointer events (ocsigen/js_of_ocaml#894) ## Bug fixes * Compiler: don't generate source if no-source-map passed (ocsigen/js_of_ocaml#780) * Compiler: Fix compilation of [Array.set] to return [unit]/0 (ocsigen/js_of_ocaml#792) * Compiler: Fix assertion failure (ocsigen/js_of_ocaml#828) * Compiler: Fix compilation of exception handlers (ocsigen/js_of_ocaml#830) * Compiler: Fix static evaluation of caml_equal (ocsigen/js_of_ocaml#906) * Misc: Fix install on windows (ocsigen/js_of_ocaml#794) * Lib: Fix Dom_svg.createForeignObject (ocsigen/js_of_ocaml#756) * Runtime: Fix caml_obj_tag, causing miscompilation with lazy value (ocsigen/js_of_ocaml#772) * Runtime: Fix caml_ml_seek_out, caml_ml_pos_out (ocsigen/js_of_ocaml#779) (Shachar Itzhaky) * Runtime: caml_parse_sign_and_base to support unsigned syntax (ocsigen/js_of_ocaml#792) (Shachar Itzhaky) * Runtime: fix encoding when printing to stdout (ocsigen/js_of_ocaml#800) * Runtime: Handle browserfs in fs_node detection logic (ocsigen/js_of_ocaml#831) * Runtime: fix Obj.tag (ocsigen/js_of_ocaml#832) * Runtime: fix marshalling of custom blocks (ocsigen/js_of_ocaml#861) * Runtime: fix frexp * Runtime: fix float printing with "%f" and large floats

Runtime: more efficient (space-wise) marshalling.

794b3e6

Modified caml_output_val to correctly reflects sharing. As a result, it avoids duplication and saves a lot of space.

Runtime: added test for marshal.

af0903b

Test sharing and cyclic data.

corwin-of-amber force-pushed the master branch from c54cca2 to af0903b Compare June 10, 2019 20:34

Runtime: Use WeakMap in marshal.

87085e9

Where available; fallback to list is used for pre-2015 envs.

corwin-of-amber commented Jun 11, 2019

View reviewed changes

Runtime: support Marshal flags.

2709d6d

Only Marshal.No_sharing makes sense in this context. Marshal.Closures cannot be supported currently. Marshal.Compat_32 is meaningless since all JavaScript integers are 32 bit.

hhugo reviewed Jun 13, 2019

View reviewed changes

Comment thread runtime/marshal.js Outdated

hhugo reviewed Jun 13, 2019

View reviewed changes

Comment thread compiler/tests/std_marshal.ml Outdated

hhugo added 2 commits June 15, 2019 12:41

tuning

9ddd401

comment

e5d7cd3

more tuning.

68e11d7

hhugo merged commit f9f91e7 into ocsigen:master Jun 15, 2019

hhugo mentioned this pull request Nov 10, 2019

[new release] js_of_ocaml, js_of_ocaml-ocamlbuild, js_of_ocaml-ppx_deriving_json, js_of_ocaml-tyxml, js_of_ocaml-ppx, js_of_ocaml-lwt, js_of_ocaml-compiler and js_of_ocaml-toplevel (3.5.0) ocaml/opam-repository#15253

Merged

bclement-ocp mentioned this pull request Mar 18, 2024

Use native big integers janestreet/zarith_stubs_js#11

Merged

Conversation

corwin-of-amber commented Jun 10, 2019

Uh oh!

hhugo commented Jun 10, 2019

Uh oh!

vouillon commented Jun 10, 2019

Uh oh!

corwin-of-amber commented Jun 10, 2019

Uh oh!

hhugo commented Jun 10, 2019

Uh oh!

corwin-of-amber commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corwin-of-amber commented Jun 10, 2019

Uh oh!

hhugo commented Jun 10, 2019

Uh oh!

corwin-of-amber commented Jun 10, 2019

Uh oh!

hhugo commented Jun 10, 2019

Uh oh!

corwin-of-amber commented Jun 10, 2019

Uh oh!

hhugo commented Jun 11, 2019

Uh oh!

corwin-of-amber commented Jun 11, 2019

Uh oh!

corwin-of-amber Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

hhugo Jun 15, 2019

Choose a reason for hiding this comment

Uh oh!

corwin-of-amber commented Jun 12, 2019

Uh oh!

hhugo Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

corwin-of-amber Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

hhugo Jun 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hhugo commented Jun 15, 2019

Uh oh!

corwin-of-amber commented Jun 15, 2019

Uh oh!

hhugo commented Jun 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

corwin-of-amber commented Jun 10, 2019 •

edited

Loading