ocsigen/js_of_ocaml

[BUG] Regression between 5.0.1 and 5.1.0+

WardBrian opened this issue · 4 comments

Describe the bug
I am a maintainer of https://github.com/stan-dev/stanc3 which uses this project to create a Javascript library https://github.com/stan-dev/stanc3/blob/master/src/stancjs/stancjs.ml

We currently are using 4.1.0, but the recent dead code elimination changes prompted me to look into upgrading to try to decrease the size of our final library. Unfortunately, this causes our existing tests to fail. I have managed to pin down the issue as being introduced in between 5.0.1 and 5.1.0.

Several of our tests are now raising a Node TypeError where they were not before:

 |$ node warnings.js
-|[]
-|["Warning in 'string', line 10, column 11: Found int division:\n      x / w\n    Values will be rounded towards zero. If rounding is not desired you can\n    write\n    the division as\n      x * 1.0 / w\n    If rounding is intended please use the integer division operator %/%."]
+|undefined
+|undefined
+|/home/brian/Dev/ml/stanc3/_build/default/src/stancjs/stancjs.bc.js:6430
+|     throw err;
+|     ^
+|
+|TypeError: Cannot read properties of undefined (reading 'length')
+|    at Object.<anonymous> (/home/brian/Dev/ml/stanc3/_build/default/test/stancjs/warnings.js:59:43)
+|    at Module._compile (node:internal/modules/cjs/loader:1275:14)
+|    at Module._extensions..js (node:internal/modules/cjs/loader:1329:10)
+|    at Module.load (node:internal/modules/cjs/loader:1133:32)
+|    at Module._load (node:internal/modules/cjs/loader:972:12)
+|    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
+|    at node:internal/main/run_main_module:23:47
+|
+|Node.js v19.9.0

If I can help narrow this down more please let me know!

Versions
ocamlc 4.14.1, js_of_ocaml 5.1.0 through 5.5.2

hhugo commented

Try this

diff --git a/src/stancjs/stancjs.ml b/src/stancjs/stancjs.ml
index 4c806f89..a2f2d116 100644
--- a/src/stancjs/stancjs.ml
+++ b/src/stancjs/stancjs.ml
@@ -248,4 +248,4 @@ let dump_stan_math_distributions () =
 let () =
   Js.export "dump_stan_math_signatures" dump_stan_math_signatures;
   Js.export "dump_stan_math_distributions" dump_stan_math_distributions;
-  Js.export "stanc" stan2cpp_wrapped
+  Js.export "stanc" (Js.Unsafe.callback stan2cpp_wrapped)

#1340 changed calling convention a bit.
You should do the same with other exported functions

hhugo commented

Could you report any size improvement using jsoo 5.5.2 ? (don't forget to build with dune --profile release)

hhugo commented

A quick check shows a 3% decrease in size.

That fixed it up, thanks!

The size went from 1873308 bytes to 1608773 on my machine, so more like 15%!