Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

De-dupe JSX elements when serializing #1150

Open
NTillmann opened this issue Nov 14, 2017 · 2 comments
Open

De-dupe JSX elements when serializing #1150

NTillmann opened this issue Nov 14, 2017 · 2 comments
Assignees

Comments

@NTillmann
Copy link
Contributor

The identity of React elements (objects with a properly configured $$typeof property) is immaterial.
We can therefore identify structurally equivalent react elements, ultimately resulting in improved code density.
Action plan:

  1. Rewrite the heap to identify structurally equivalent react elements, e.g. in the ResidualHeapVisitor, by maintaining a new HashSet containing wrapper objects around react elements that implement a structural getHash and equals.
    (Note that as a result, a react element may now appear in multiple scopes.)
  2. For children of react elements, the serializer currently has a special treatment in _serializeValueReactElementChild: it bypasses the regular logic of serializeValue that hands out names to values and selects the proper target body. This needs to get revisited. One approach would be to just fall back to the regular code path. This would result in aesthetically displeasing code, unless the option --inlineExpressions is given; however, when combined with an JavaScript compiler that produces bytecode, the resulting bytecode is likely the same even without --inlineExpressions.

The reason _serializeValueReactElementChild currently does what it does is to add keys to array elements. However, @trueadm has a plan how to switch from arrays to fragments which no longer have that requirement. We'll probably need that first.

@trueadm trueadm changed the title De-dupe JSC elements when serializing De-dupe JSX elements when serializing Nov 14, 2017
@trueadm
Copy link
Contributor

trueadm commented Nov 14, 2017

One constraint to add, if the JSX attribute of key is a StringValue we cannot add these de-dupe optimizations to that React element as this depends on React runtime constraint. Check this issue for prior discussions on this topic: facebook/react#3226

So this can be optimized:

function MyComponent() {
  return (
    <div>
      <span />
      <span />
      <span />
      <span />
    </div>
  );
}

To be:

const __a = <span />;
const __b = <div>{__a}{__a}{__a}{__a}</div>;

function MyComponent() {
  return __b;
}

If a React element has a key, we will also need to preserve that. For example:

function MyComponent() {
  return [<div key="a" />, <div key="b" />, <div key="c" />, <div key="d" />];
}

We can't hoist those nodes reliably, one way we could optimize this could be:

__div(key) {
  return <div key={key} />
}
function MyComponent() {
  return [__div(a), __div(b), __div(c), __div(d)];
}

facebook-github-bot pushed a commit that referenced this issue Nov 14, 2017
Summary:
After adding some tests, it turns out that logic to apply keys to arrays was un-needed. This was because, after running the same code through React, the same errors occur when keys are omitted from arrays. We don't need to do any clever tricks to add keys, as it might actually break logic on things that don't need them. I've added the relevant tests and removed the key adding logic, which should help us with the de-duping work (#1150).
Closes #1151

Differential Revision: D6323878

Pulled By: trueadm

fbshipit-source-id: ec3fa90c48332ef0715e94335c558570040ed81c
@trueadm
Copy link
Contributor

trueadm commented Dec 1, 2017

A bunch of these use-cases should be covered by the PR: #1196

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

No branches or pull requests

2 participants