Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Regression between 5.0.1 and 5.1.0+ #1543

Closed
WardBrian opened this issue Dec 7, 2023 · 4 comments
Closed

[BUG] Regression between 5.0.1 and 5.1.0+ #1543

WardBrian opened this issue Dec 7, 2023 · 4 comments
Labels

Comments

@WardBrian
Copy link

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

@WardBrian WardBrian added the bug label Dec 7, 2023
@hhugo
Copy link
Member

hhugo commented Dec 8, 2023

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
Copy link
Member

hhugo commented Dec 8, 2023

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

@hhugo
Copy link
Member

hhugo commented Dec 8, 2023

A quick check shows a 3% decrease in size.

@WardBrian
Copy link
Author

That fixed it up, thanks!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants