Skip to content

Use javascript string by default #976

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

Merged
merged 2 commits into from
Feb 3, 2023
Merged

Use javascript string by default #976

merged 2 commits into from
Feb 3, 2023

Conversation

hhugo
Copy link
Member

@hhugo hhugo commented Apr 3, 2020

In that mode, an OCaml string will be represented as a JavaScript string containing a sequence of bytes. Converting the string to utf16 is still required for interaction with the DOM, or other javascript API.

This current alternative representation of strings (implemented in #923) is not set in stone and could be changed based on benchmarks and usability.

Here are a bunch of things to consider:

  • Wrap JavaScript string inside an object (MlString) to ease debugging.
  • Update string.ml to make more optimization possible (String.sub, (^), ..)
  • Add more optimization in jsoo itself recognize String concat #977

The PR is useful to check that existing program don't make any assumption about the internal representation of strings.

@hhugo hhugo force-pushed the jsstring-enable branch 3 times, most recently from 0eeebae to fdc7198 Compare April 4, 2020 13:03
@hhugo hhugo changed the title Use javascript string by default [WIP] Use javascript string by default Apr 7, 2020
@hhugo hhugo force-pushed the jsstring-enable branch 2 times, most recently from d177ba0 to 4b149ee Compare April 9, 2020 14:08
@Drup
Copy link
Member

Drup commented Apr 10, 2020

I know this is WIP, but I'm curious. What would be the consequence for users, notably in term of FFI (and performances) ?

@hhugo
Copy link
Member Author

hhugo commented Apr 10, 2020

This is not really a WIP anymore. It's being tested now.

There should be no implication for FFI. One still need to use existing primitive/stubs to convert between js (utf16) and ocaml (utf8) strings.

I would expect some performances improvement but I didn't run any bench yet.

@hhugo hhugo marked this pull request as ready for review April 10, 2020 16:38
@Drup
Copy link
Member

Drup commented Apr 10, 2020

Hmm, maybe I misunderstood then ! If it's neither for FFI nor perf, what's the goal ? :)

@hhugo
Copy link
Member Author

hhugo commented Apr 10, 2020

It would be for perf ! We still need to run some bench.

@hhugo
Copy link
Member Author

hhugo commented Apr 10, 2020

(#655 contains all the logic and I believe does some nice cleanup)

@hhugo hhugo force-pushed the jsstring-enable branch 3 times, most recently from 0325ad6 to 64864fd Compare April 14, 2020 09:57
@hhugo hhugo added the blocked label Apr 14, 2020
@hhugo
Copy link
Member Author

hhugo commented Apr 18, 2020

@ejgallego, would you be able to test jscoq against this GPR ?

@ejgallego
Copy link
Contributor

@ejgallego, would you be able to test jscoq against this GPR ?

Hi @hhugo , sorry I missed this. Yes, we can test soon.

@ejgallego
Copy link
Contributor

ejgallego commented Apr 23, 2020

@hhugo I get "caml_jsstring_of_string not implemented", I was able to fix that by adding stuff in mlbytes and bigarray.js manually; I got a bit far but still seeing some problems in the de-serialization of objects, likely due to the hacks we have there.

@hhugo hhugo force-pushed the jsstring-enable branch from 64864fd to a993312 Compare April 23, 2020 15:54
@hhugo
Copy link
Member Author

hhugo commented Apr 23, 2020

I don't understand why you get "caml_jsstring_of_string not implemented". Have you pin both js_of_ocaml and js_of_ocaml-compiler to this PR ?

@ejgallego
Copy link
Contributor

ejgallego commented Apr 23, 2020

I did pin a bunch of libs, let me re-pin everything again, indeed maybe I forgot to pin -compiler

@ejgallego
Copy link
Contributor

Ok, now I got:

Uncaught TypeError: Cannot read property 'charCodeAt' of undefined

in Coq itself. Will switch to debug mode and research more.

@hhugo
Copy link
Member Author

hhugo commented Apr 23, 2020

I can help you if you need. Note that marshal on bytes will not work properly.

@hhugo
Copy link
Member Author

hhugo commented Apr 23, 2020

@ejgallego
Copy link
Contributor

ejgallego commented Apr 23, 2020

Very well spotted indeed (cc: @corwin-of-amber )

Uncaught TypeError: Cannot read property 'charCodeAt' of undefined
    at re_match_impl (:8080/coq-js/jscoq_worker.bc.js:7000)
    at re_string_match (:8080/coq-js/jscoq_worker.bc.js:7783)
    at string_match$0 (:8080/coq-js/jscoq_worker.bc.js:125117)
    at bounded_split$0 (:8080/coq-js/jscoq_worker.bc.js:125343)
    at split$5 (:8080/coq-js/jscoq_worker.bc.js:125393)
    at split_flags (:8080/coq-js/jscoq_worker.bc.js:151618)
    at flags_of_string (:8080/coq-js/jscoq_worker.bc.js:151709)
    at parse_flags (:8080/coq-js/jscoq_worker.bc.js:151765)
    at set_flags (:8080/coq-js/jscoq_worker.bc.js:151783)
    at create$31 (:8080/coq-js/jscoq_worker.bc.js:151859)

that should be easy to patch by myself.

@ejgallego
Copy link
Contributor

With that patched, jsCoq demo file seems to work fine!

@hhugo hhugo added this to the 3.7 milestone Apr 24, 2020
@hhugo hhugo removed the blocked label Apr 24, 2020
@hhugo hhugo force-pushed the jsstring-enable branch from e285644 to 482a91b Compare April 25, 2020 11:52
@hhugo
Copy link
Member Author

hhugo commented Apr 25, 2020

Rebased on top of the new Str support.

@dbuenzli
Copy link
Contributor

I share @Drup's curiosity with thrill. Do you have a design doc somewhere that describes the move and its implications ? The JavaScript vs OCaml strings has been an annoying technical point of jsoo when designing OCaml APIs that interact with the browser APIs.

Does it mean that if I have a value of type string it is represented by a JavaScript string and I can give it to the DOM apis as is ? And how are string literals treated ?

@hhugo
Copy link
Member Author

hhugo commented Apr 25, 2020

Does it mean that if I have a value of type string it is represented by a JavaScript string and I can give it to the DOM apis as is ? And how are string literals treated ?

In that mode, an ocaml string will be represented as a javascript string containing a sequence of bytes. It would only be safe to pass it to the dom as is for ascii strings. In the general case, you'll still need to convert the string to utf16.

I will update the feature description to explain the design and the plan moving forward.

In short:

  • A previous PR Compiler: compile ocaml string as javascript ones #923 made changing strings representation easier by cleaning up the runtime
  • The alternative string representation is currently a plain js string containing bytes
  • The plan is to run multiple bench with different implementations, eventually updating the implementation of string.ml and take a decision based on the numbers.

@hhugo hhugo force-pushed the jsstring-enable branch from 482a91b to 7dbe371 Compare April 25, 2020 13:54
@dbuenzli
Copy link
Contributor

Okay so the dichotomy remains. But I guess given the nature of OCaml strings -- namely a sequence of bytes -- there's not much choice.

@hhugo
Copy link
Member Author

hhugo commented Nov 9, 2020

one more step, num will be compatible with that change ocaml/num#21

@hhugo hhugo removed this from the 3.7 milestone Nov 12, 2020
@smorimoto
Copy link
Member

At least #984 have been merged.

@hhugo hhugo force-pushed the jsstring-enable branch 2 times, most recently from e4ce045 to 30beca9 Compare January 12, 2023 18:41
@hhugo hhugo merged commit eae3486 into master Feb 3, 2023
@hhugo hhugo deleted the jsstring-enable branch February 3, 2023 13:07
@smorimoto
Copy link
Member

Is there any bench somewhere for this?

@hhugo
Copy link
Member Author

hhugo commented Feb 3, 2023

https://ocsigen.org/js_of_ocaml/latest/manual/performances was updated with some numbers.

@smorimoto
Copy link
Member

Great! Thanks for the pointer!

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

Successfully merging this pull request may close these issues.

5 participants