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

[Experiment][DoNotmerge] Represent ocaml records as js objects at runtime. #2639

Closed

Conversation

cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented Mar 16, 2018

Experiment with representing ocaml records as js objects at runtime, to assess the number of changes involved to support a debug mode where the field names are visible.

For example let a = {x=12; y=34} becomes

var a = /* record */{
  x: 12,
  y: 34
};

Field access and update are also supported.

This PR changes the core functions plus a few cases that rely on the assumption that arrays are used at runtime.

A few known missing things are:

  • generic compare always returns 0, as it expects blocks with tag and length
  • accessing the content of refs with .contents does not work
  • the lexer assumes an array representation
  • surely more

But for self-contained code, it’s possible to run some experiments already.

Experiment with representing ocaml records as js objects at runtime, to assess the number of changes involved to support a debug mode whether the field names are visible.

For example `let r = {x=12; y=34}` becomes
```
var a = /* record */{
  x: 12,
  y: 34
};
```

Field access and update are also supported.

This PR changes the core functions plus a few cases that rely on the assumption that arrays are used at runtime.

A few known missing thigs are:
- generic compare always returns 0, as it expects blocks with tag and length
- accessing the content of refs with .contents does not work
- the lexer assumes an array representation
- surely more
But for self-contained code, it’s possible to run some experiments already.
@chenglou
Copy link
Member

generic compare always returns 0, as it expects blocks with tag and length
ok, time to re-enable that polymorphic comparison warning; but I think before that we need to have some good equality comparison generation support; otherwise this is a footgun.

But this is a feature I'd pay money for

@bobzhang
Copy link
Member

The ref should be easy to fix, it involves two primitives. The challenging part is there are lots of the code in the wild (in compiler internals and user land code) (using Obj.magic ) that assumes the runtime representation between record and tuple is the same

@anru
Copy link

anru commented Mar 21, 2018

Personally, I very like feature of compressing records to just arrays in JS runtime, which works faster and has smaller ship code size, while objects/records are heavily used in common frontend codebase

@rafayepes
Copy link

That's probably a terrible idea, but could we have 2 compilation modes? One for better interop and another for better perf

@chenglou
Copy link
Member

We'll need such thing eventually, but I don't think it impacts this specific feature. The goal would be to use the actual underlying representation (js object) for more than just development purposes, e.g. to avoid the need for record<->js obj conversion, to type json objects as records, etc.

@flash-gordon
Copy link

There is no evidence so far using objects is slower, if anyone has any please attach

@chenglou
Copy link
Member

chenglou commented Mar 22, 2018

There's an entire thread on it: #24

@bmeurer any change from V8's side? Right now the extra consideration is interop, not just perf. So tending toward compiling records to js objs

@bmeurer
Copy link
Contributor

bmeurer commented Mar 22, 2018

You need less metadata for arrays. Otherwise I'm not entirely sure what are the implications of this change. So hard to tell. Intuitively objects will be faster on access.

Cristiano Calcagno added 4 commits March 22, 2018 19:13
References are hardcoded early on to use arrays. Special-case access to the “contents” field to use array access.
The 3 cases in this function are collapsed into one:
```
let getval =
  let f x = x+1 in
  function
  | A {a} -> f a
  | B {b} -> f b
  | C {c} -> f c

```
This is because the index access in the 3 cases is the same: the first index.
When switching to the object representation, this optimization is unsound.

Enriched comparison of lambda primitives to distinguish access to fields with different names, even though they might have the same index.

With this, all tests pass.
@quicksnap
Copy link

Thank you so much for putting work into this. I really, really appreciate your effort!

Linking to #2556; related to meaningful printing. Having these kind of dev-time changes would make debugging wonderful.

@cloudkite
Copy link

cloudkite commented Apr 5, 2018

would this mean we no longer need todo things like?

[@bs.deriving jsConverter]
type thing = {
  stuff: int,
};

let thing = thingFromJs({"stuff": 34})

If so this would be really nice for js interop 🙏 🎉

As someone trying out reason & BuckleScript it was a very jarring experience having to choose between using the less idiomatic %bs.obj OR using Record and then having to convert between the two. It also felt like a bit of mental overhead having a mixture of thing##stuff and otherThing.stuff. Debugging a code base that uses Records extensively seemed like it would be cryptic when everything is an array.

This was just my initial impressions from writing a simple hello world app, maybe it's not a big deal in practice 🤷‍♀️ . But it made me a bit nervous about writing a larger app with reason & BuckleScript.

@joshburgess
Copy link

This would be really useful for practical things, such as integrating with dev tools. See: rickyvetter/reductive#30

@bobzhang
Copy link
Member

@joshburgess hi, we have some solutions for dev tools coming soon

@joshburgess
Copy link

@bobzhang Cool, looking forward to that. 👍

@cristianoc
Copy link
Collaborator Author

In master.

@cristianoc cristianoc closed this Nov 6, 2019
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.

10 participants