Optimizations not applied when record has an optional field

[cknitt]

The following code (without an optional field)

type t = {
  a: string,
  b: int
}

let x = { a: "test", b: 1 }
let y = { ...x, b: 5 }

let f = x => { ...x, b: x.b + 1 }

is compiled to

var y = {
  a: "test",
  b: 5
};

function f(x) {
  return {
          a: x.a,
          b: x.b + 1 | 0
        };
}

var x = {
  a: "test",
  b: 1
};

However, as soon as I make a an optional field, the output changes to the more inefficient

var x = {
  a: "test",
  b: 1
};

var newrecord = Caml_obj.obj_dup(x);

newrecord.b = 5;

function f(x) {
  var newrecord = Caml_obj.obj_dup(x);
  newrecord.b = x.b + 1 | 0;
  return newrecord;
}

var y = newrecord;

[cristianoc]

What about other types than optional? E.g. array or list or tuple etc.

[cknitt]

Array does not cause Caml_obj.obj_dup to be used:

type t = {
  a: array<string>,
  b: int
}

let x = { a: ["test"], b: 1 }
let y = { ...x, b: 5 }

let f = x => { ...x, b: x.b + 1 }

->

var x_a = ["test"];

var x = {
  a: x_a,
  b: 1
};

var y_a = x_a;

var y = {
  a: y_a,
  b: 5
};

function f(x) {
  return {
          a: x.a,
          b: x.b + 1 | 0
        };
}

[cristianoc]

So this does not produce obj_dup:

type t = {
  a: string,
  b: option<int>
}

let f = x => { ...x, a:"" }

but this does:

type t = {
  a: string,
  b?: int
}

let f = x => { ...x, a:"" }

suggesting the different type representation triggers a different object copy heuristic.

[cristianoc]

I believe this heuristic is intentional, and applied to objects with sparse fields. That's all I know about it.

[cristianoc]

CC @bobzhang

[cristianoc]

I believe here: https://github.com/rescript-lang/rescript-compiler/pull/5253/files#diff-0198658e54fcaee976985a7b573751510182591849d8d1da9a7aac8e2539f8e1R989

[cristianoc]

@cknitt I think what needs to be understood here is:

1. typical uses of records with optional fields
2. performance characteristics of classes of uses on common JS engines

From that info, one could then refine the heuristics.

The more difficult one at the moment seems 1 as the feature is new. However, one can take component-heavy code with JSX V4 as one class of use (add double check that record update is in fact used there -- can't remember for sure).

[TheSpyder]

I was about to say this seems bad, but given that obj_dup is just an isArray check and a single Object.assign: https://github.com/rescript-lang/rescript-compiler/blob/6de8e49f02185b5abd54803cfa781df20d3a85bf/lib/es6/caml_obj.js#L20

This actually seems fine. For applications using a JS bundler it will probably be less code than the "cleaner" copy process for normal records.

And long term, once we can use ES6 syntax, we can swap the obj_dup call for the JS spread operator as I believe they have the same semantics (although I haven't read the spec).

[bobzhang]

I believe this heuristic is intentional,

Yes, the feature of "optional field" was designed for configurations where performance should not matter.

[cknitt]

Ok, so basically it works as designed - shall I close the issue or shall we keep it open until we have more experience with typical usage, especially with JSX v4?

[cristianoc]

Added it to the TBD milestone 10.3.

[cristianoc]

@cknitt what's the conclusion on this?

[cknitt]

AFAIK Object.assign is not very performant, so I would still be happy to see optimizations being applied for records with optional fields, too. However, the problem I see is that if we enabled the same heuristic (create new record with given fields if number of fields < 20) for records with optional fields, too, and

type t = {
  a: string,
  b?: int
}

let f = x => { ...x, a:"" }

was compiled to

function f(x) {
  return {
          a: "",
          b: x.b
        };
}

then we'd end up transforming {a: "something"} to {a: "", b: undefined} which I think is not what we want.

We'd probably want to generate something like

function f(x) {
  var newrecord = {a: ""};
  if (x.b !== undefined) {
    newrecord.b = x.b;
  }
  return newrecord;
}

[cristianoc]

Sounds like there's plenty of open questions here. Moving to 12. If quick progress is made converging on a solution, it can be moved back to 11.