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

Optimizing Compiler: Inline ReactElements #3228

Closed
sebmarkbage opened this Issue Feb 22, 2015 · 22 comments

Comments

Projects
None yet
7 participants
@sebmarkbage
Member

sebmarkbage commented Feb 22, 2015

Starting with React 0.14 we will be able to inline ReactElements:

<div className="foo">{bar}<Baz key="baz" /></div>

as objects:

{ type: 'div', props: { className: 'foo', children:
  [ bar, { type: Baz, props: { }, key: 'baz', ref: null } ]
}, key: null, ref: null }

This improves performance over the existing React.createElement call by inlining the result of it.

defaultProps

If a component might have default props they need to be resolved by the transpiler's runtime:

{ type: 'div', props: { className: 'foo', children:
  [ bar, { type: Baz, props: $resolveDefaults(Baz.defaultProps, { }), key: 'baz', ref: null } ]
}, key: null, ref: null }

Exception: ref="string"

Unfortunately we still haven't figured out what the final semantics for refs. The current semantics relies on getting the current React owner. Therefore, we cannot apply this optimization if the ref attribute might be a string.

render() {
  // Neither of these...
  return <div ref="str" />;
  // ...are safe to inline...
  return <div ref={possibleStringValue} />;
  // ...because they might contain a ref.
  return <div {...objectThatMightContainARef} />;
}

Non-JSX

This can work on React.createElement or functions created by React.createFactory if the first argument is an inline object literal. Otherwise it is not safe since the object might be reused and mutated.

Only in Production Mode

This optimization should only be applied in production mode. Currently React.createElement fires various warnings for propTypes and key warnings if the __DEV__ flag is set to true or "production" !== process.env.NODE_ENV. This optimization would skip the warnings which would be very very bad in development mode.

The difficult part of this is figuring out a way that this will work in everyone's environment because not everyone has the ability to use different transpilers for development and production mode.

One solution might be to use a ternary and rely on minifiers to strip out the extra code:

"production" === process.env.NODE_ENV ?
  { type: 'div', props: { ... }, key: null, ref: null } :
  React.createElement('div', ...)

This will a pain for source maps though.

Another solution would be to have different flags in the transpilers themselves but we'd have to make sure that people actually use them correctly. They will otherwise have problems due to not firing warnings, or think that React is slow because they screwed up their config.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
Member

sebmarkbage commented Feb 22, 2015

cc @sebmck

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 22, 2015

Member

The difficult part of this is figuring out a way that this will work in everyone's environment because not everyone has the ability to use different transpilers for development and production mode.

What do you mean by using different transpilers? You'd just be using the same transpiler flick a switch unless I'm misunderstanding something?

Another solution would be to have different flags in the transpilers themselves but we'd have to make sure that people actually use them correctly. They will otherwise have problems due to not firing warnings, or think that React is slow because they screwed up their config.

Developers already know to use a different build of React for development and production so I don't think it'll be confusing to introduce the same concept to the transpilation step. I've been toying with the idea of having a production and development mode in Babel as there are things like TDZ that I really want to enable by default but would absolutely kill performance in hot code and their only purpose is to basically prevent development errors.

Member

kittens commented Feb 22, 2015

The difficult part of this is figuring out a way that this will work in everyone's environment because not everyone has the ability to use different transpilers for development and production mode.

What do you mean by using different transpilers? You'd just be using the same transpiler flick a switch unless I'm misunderstanding something?

Another solution would be to have different flags in the transpilers themselves but we'd have to make sure that people actually use them correctly. They will otherwise have problems due to not firing warnings, or think that React is slow because they screwed up their config.

Developers already know to use a different build of React for development and production so I don't think it'll be confusing to introduce the same concept to the transpilation step. I've been toying with the idea of having a production and development mode in Babel as there are things like TDZ that I really want to enable by default but would absolutely kill performance in hot code and their only purpose is to basically prevent development errors.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 22, 2015

Member

What do you mean by using different transpilers? You'd just be using the same transpiler flick a switch unless I'm misunderstanding something?

Yea, that's what I meant.

Developers already have minifiers that they only use in production mode. This is effectively the same thing.

Member

sebmarkbage commented Feb 22, 2015

What do you mean by using different transpilers? You'd just be using the same transpiler flick a switch unless I'm misunderstanding something?

Yea, that's what I meant.

Developers already have minifiers that they only use in production mode. This is effectively the same thing.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 22, 2015

Member

Ah, right. Yeah, definently more a developer education issue than a technical one.

Member

kittens commented Feb 22, 2015

Ah, right. Yeah, definently more a developer education issue than a technical one.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 22, 2015

Member

Nit: developer education is a technical issue. :) Look at how much technical trouble we go through to add warnings and locking down the API so that developers don't unknowingly shoot themselves in the foot.

Member

sebmarkbage commented Feb 22, 2015

Nit: developer education is a technical issue. :) Look at how much technical trouble we go through to add warnings and locking down the API so that developers don't unknowingly shoot themselves in the foot.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 22, 2015

Member

Yeah, absolutely agree. Probably should have been more specific, I was referring specifically to educating developers about any potential "production" mode in transpilers since there's no real way to warn them if they've compiled their production code in development mode.

Member

kittens commented Feb 22, 2015

Yeah, absolutely agree. Probably should have been more specific, I was referring specifically to educating developers about any potential "production" mode in transpilers since there's no real way to warn them if they've compiled their production code in development mode.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Feb 22, 2015

Contributor

@sebmarkbage

Unfortunately we still haven't figured out what the final semantics for refs. The current semantics relies on getting the current React owner. Therefore, we cannot apply this optimization if the ref attribute might be a string.

There were mentions in the past of "inverting" refs, you instead provide a ref-object to ref which is updated. That sounded great to me, it gets rid of the implicit owner and possibly even the magic object... additionally you could set up a refs-object which could take care of lists of components, this does not work well today with strings.

But... what if we implemented ref as a callback(s) instead which receives the instance as an argument and something to distinguish whether it was mounted or unmounted, kind of similar to how onChange/valueLink works in a sense. That way we leave exact implementations up to the user and there is no preference for either individual instances or lists of instances nor where you store them (if you even store them). These could even be chained easily. But this is no longer a system for refs, it's basically externally chainable life-cycle method listeners which can be used to reimplement refs.

I imagine this could have other benefits in the future; expose all life-cycle callbacks and you could use it to say easily measure frontend objects without ever having to store the instance, nor provide complex logic to determine whether or not the frontend object actually updated. This exact use-case is just an idea, but this approach seems a lot more React:y, reusable and naturally efficient than refs which seems like something you should avoid unless explicitly dealing with uncontrolled components in which case you have no other option.

Perhaps I'm way off on the utility here, but it seems to me that it solves a lot of problems that refs as meant to solve but in better ways and actually storing refs should only necessary when dealing with uncontrolled components. All other uses should reasonable be capable of doing its thing directly on instance when the callback is called.

Re-implementing refs as-is could look something like the code below, it could easily be made into a reusable helper. It could take any number of shapes, perhaps a key isn't even interesting and just putting all instances in a Set is enough. You decide.

function createRef(that, name, next) {
  return {
    next: next,
    componentDidMount: function(instance) {
      that.refs[name] = instance;
    },
    componentWillUnmount: function(instance) {
      delete that.refs[name];
    }
  };
}

To keep updated measurements of a component you would simply as below, if the editor didn't update then you automatically avoid re-measuring for free. If only the editor updates, but not your own component, you still remeasure and get the updated height.

<Editor lifeCycle={{
  componentDidMount: (instance) => {
    this.setState({editorHeight: instance.measureHeight()});
  },
  componentDidUpdate: (instance) => {
    this.setState({editorHeight: instance.measureHeight()});
  }
}} />
Contributor

syranide commented Feb 22, 2015

@sebmarkbage

Unfortunately we still haven't figured out what the final semantics for refs. The current semantics relies on getting the current React owner. Therefore, we cannot apply this optimization if the ref attribute might be a string.

There were mentions in the past of "inverting" refs, you instead provide a ref-object to ref which is updated. That sounded great to me, it gets rid of the implicit owner and possibly even the magic object... additionally you could set up a refs-object which could take care of lists of components, this does not work well today with strings.

But... what if we implemented ref as a callback(s) instead which receives the instance as an argument and something to distinguish whether it was mounted or unmounted, kind of similar to how onChange/valueLink works in a sense. That way we leave exact implementations up to the user and there is no preference for either individual instances or lists of instances nor where you store them (if you even store them). These could even be chained easily. But this is no longer a system for refs, it's basically externally chainable life-cycle method listeners which can be used to reimplement refs.

I imagine this could have other benefits in the future; expose all life-cycle callbacks and you could use it to say easily measure frontend objects without ever having to store the instance, nor provide complex logic to determine whether or not the frontend object actually updated. This exact use-case is just an idea, but this approach seems a lot more React:y, reusable and naturally efficient than refs which seems like something you should avoid unless explicitly dealing with uncontrolled components in which case you have no other option.

Perhaps I'm way off on the utility here, but it seems to me that it solves a lot of problems that refs as meant to solve but in better ways and actually storing refs should only necessary when dealing with uncontrolled components. All other uses should reasonable be capable of doing its thing directly on instance when the callback is called.

Re-implementing refs as-is could look something like the code below, it could easily be made into a reusable helper. It could take any number of shapes, perhaps a key isn't even interesting and just putting all instances in a Set is enough. You decide.

function createRef(that, name, next) {
  return {
    next: next,
    componentDidMount: function(instance) {
      that.refs[name] = instance;
    },
    componentWillUnmount: function(instance) {
      delete that.refs[name];
    }
  };
}

To keep updated measurements of a component you would simply as below, if the editor didn't update then you automatically avoid re-measuring for free. If only the editor updates, but not your own component, you still remeasure and get the updated height.

<Editor lifeCycle={{
  componentDidMount: (instance) => {
    this.setState({editorHeight: instance.measureHeight()});
  },
  componentDidUpdate: (instance) => {
    this.setState({editorHeight: instance.measureHeight()});
  }
}} />
@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 22, 2015

Member

@syranide We already implemented both first-class refs and then switched them to callbacks in #2917. You haven't been keeping up. :)

It is not very convenient and very imperative since you need to keep your own reference to it. The timing of life-cycles isn't ideal too. Therefore I'm not comfortable deprecating the current refs until we're sure which model is final, and make sure that we have a good upgrade path (codemodable).

Member

sebmarkbage commented Feb 22, 2015

@syranide We already implemented both first-class refs and then switched them to callbacks in #2917. You haven't been keeping up. :)

It is not very convenient and very imperative since you need to keep your own reference to it. The timing of life-cycles isn't ideal too. Therefore I'm not comfortable deprecating the current refs until we're sure which model is final, and make sure that we have a good upgrade path (codemodable).

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Feb 22, 2015

Contributor

@sebmarkbage * facepalm * ;)

Contributor

syranide commented Feb 22, 2015

@sebmarkbage * facepalm * ;)

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Mar 29, 2015

Member

I've done a complete and stable implementation of this (see attached commit). Also works perfectly with #3226 (no real reason for it not to).

Member

kittens commented Mar 29, 2015

I've done a complete and stable implementation of this (see attached commit). Also works perfectly with #3226 (no real reason for it not to).

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Sep 3, 2015

Contributor

@sebmarkbage
Do you have any updates on the issue?

Contributor

koba04 commented Sep 3, 2015

@sebmarkbage
Do you have any updates on the issue?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Sep 10, 2015

Member

This proposal now need to be revised to include a $$typeof field as per #4832

Member

sebmarkbage commented Sep 10, 2015

This proposal now need to be revised to include a $$typeof field as per #4832

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Sep 10, 2015

Member

@sebmarkbage shh I was working on a PR! now @sebmck will beat me for sure…

Member

sophiebits commented Sep 10, 2015

@sebmarkbage shh I was working on a PR! now @sebmck will beat me for sure…

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@elsassph

This comment has been minimized.

Show comment
Hide comment
@elsassph

elsassph Apr 21, 2016

Performance question: is it preferable to always include the key: null, ref: null or is it ok to omit those if they are undefined? eg. maybe browsers can better optimize the code if the shape of objects is always the same.

Performance question: is it preferable to always include the key: null, ref: null or is it ok to omit those if they are undefined? eg. maybe browsers can better optimize the code if the shape of objects is always the same.

@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Apr 21, 2016

Contributor

@elsassph keeping those properties allows JIT compilers (such as V8) to match the "hidden class" of the objects. Generally speaking this should give better performance.

Contributor

trueadm commented Apr 21, 2016

@elsassph keeping those properties allows JIT compilers (such as V8) to match the "hidden class" of the objects. Generally speaking this should give better performance.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Apr 21, 2016

Contributor

@trueadm You'll benefit from that anyway, there will just be more "hidden classes". I suspect there's nothing to be gained in this context, the performance difference is small and spread over the whole project it's unlikely to even be measurable and if you overdo it you're actually increasing the cost as a result of dealing with larger objects and more processing involved in creating them (again probably immeasurably). So I would say it's largely counter-productive, if you're rendering large lists it may be a good idea to keep the signature the same (so the hidden class is kept in fast cache), but even then I doubt it's really measurable.

Contributor

syranide commented Apr 21, 2016

@trueadm You'll benefit from that anyway, there will just be more "hidden classes". I suspect there's nothing to be gained in this context, the performance difference is small and spread over the whole project it's unlikely to even be measurable and if you overdo it you're actually increasing the cost as a result of dealing with larger objects and more processing involved in creating them (again probably immeasurably). So I would say it's largely counter-productive, if you're rendering large lists it may be a good idea to keep the signature the same (so the hidden class is kept in fast cache), but even then I doubt it's really measurable.

@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Apr 21, 2016

Contributor

@syranide The performance can definitely make an impact. As you point out, it's very good for large lists of virtual nodes, where you need to aggressively diff them. This is one of the many reasons why Inferno gets very good performance, especially when it comes to complex node tress that rapidly update.

Keeping object shapes monomorphic can give significant benefits in regards to V8 and how it decides what to deopt (I've spent countless hours going through the ASM output using IRHydra2 checking for such cases).

Contributor

trueadm commented Apr 21, 2016

@syranide The performance can definitely make an impact. As you point out, it's very good for large lists of virtual nodes, where you need to aggressively diff them. This is one of the many reasons why Inferno gets very good performance, especially when it comes to complex node tress that rapidly update.

Keeping object shapes monomorphic can give significant benefits in regards to V8 and how it decides what to deopt (I've spent countless hours going through the ASM output using IRHydra2 checking for such cases).

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Apr 21, 2016

Contributor

The performance can definitely make an impact. As you point out, it's very good for large lists of virtual nodes...

Generally there is a single code path being repeated and it's a very unusual setup if you're mixing keys with no keys and refs with no refs in a list.

Keeping object shapes monomorphic can give significant benefits...

Yes, but preemptively adding key: null, ref: null is unlikely to do that. It only matters if you're creating elements that otherwise have the exact same signature. This is very uncommon for React elements in my experience, use of ref is frowned upon and rarely used, use of key is largely related to the intent of the component. If you were to print out all the unique React element signatures before and after doing this for all of them I'm quite confident the difference would be very small and the actual impact immeasurable. There's nothing wrong with reducing hidden classes, but preemptively adding null key/ref seems entirely counter-productive as a general rule.

Contributor

syranide commented Apr 21, 2016

The performance can definitely make an impact. As you point out, it's very good for large lists of virtual nodes...

Generally there is a single code path being repeated and it's a very unusual setup if you're mixing keys with no keys and refs with no refs in a list.

Keeping object shapes monomorphic can give significant benefits...

Yes, but preemptively adding key: null, ref: null is unlikely to do that. It only matters if you're creating elements that otherwise have the exact same signature. This is very uncommon for React elements in my experience, use of ref is frowned upon and rarely used, use of key is largely related to the intent of the component. If you were to print out all the unique React element signatures before and after doing this for all of them I'm quite confident the difference would be very small and the actual impact immeasurable. There's nothing wrong with reducing hidden classes, but preemptively adding null key/ref seems entirely counter-productive as a general rule.

@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Apr 21, 2016

Contributor

@syranide It looks great on benchmarks if done right, but in the grand scheme of things, it's unlikely to make much difference as the app code the user has created is most likely to be the bottleneck in any case, but it's worth having if you can get it for free at a compiler level.

Contributor

trueadm commented Apr 21, 2016

@syranide It looks great on benchmarks if done right, but in the grand scheme of things, it's unlikely to make much difference as the app code the user has created is most likely to be the bottleneck in any case, but it's worth having if you can get it for free at a compiler level.

@elsassph

This comment has been minimized.

Show comment
Hide comment
@elsassph

elsassph Apr 21, 2016

Thanks that's the kind of considerations I'm facing - making objects smaller by not including null properties VS shadow classes potential benefits.

Thanks that's the kind of considerations I'm facing - making objects smaller by not including null properties VS shadow classes potential benefits.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Apr 22, 2016

Member

On the React team we've been operating under the assumption that it's better to include the nulls.

I'm going to close this since the babel plugin now exists.

Member

sophiebits commented Apr 22, 2016

On the React team we've been operating under the assumption that it's better to include the nulls.

I'm going to close this since the babel plugin now exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment