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

What the optimization caveats of using stateless function without shouldComponentUpdate = false? #163

Closed
idchlife opened this issue May 26, 2016 · 33 comments

Comments

@idchlife
Copy link

I'm wondering, if I have >30 stateless function components only with different props (callbacks, just values) that changes or does not change (one time show with props and stays as it is) - will there be penalty because I'm not making class-based components with check for shouldComponentUpdate?

@developit
Copy link
Member

Hi @IDCH - currently this would mean incurring the diff cost, as you suspected. However, I'm working on an equivalent to shouldComponentUpdate():false here and would love some input. The idea is that Preact would retain the previous VNode (the tree really) returned by each stateless functional component, and the diff would check for an exact match on a subsequent call and skip the diff entirely (same optimization as shouldComponentUpdate).

The key here would be that in order to actually produce the same VNode on subsequent invocations, you'd have to memoize the pure function.

Another alternative would be to make the assumption that all stateless functional components are pure. I know for my own work this is true, but I'm not sure if there would be cases that break here. If we could assume pure functions, then the optimization would be pretty simple (same props as last call skips the diff).

Thoughts?

@idchlife
Copy link
Author

@developit by pure functions you mean that they don't modify arguments they got and does not activate some side effects like 'exactly every time parent component renders it should notify something about it';

I think actually that this could be added to documentation - that pure functions are always based on props diff and do not render when there is no difference between props.

It means opposite from as now - if you want re-renders, use class based components without shouldComponentUpdate. Basically stateless functions become like 'component with shouldComponentUpdate false'.

I did not yet work with insides of preact, and kinda short on time but I'll try to look into this to help. Or did you want help only as discussion?

@developit
Copy link
Member

Just pure in the functional sense: multiple calls with the same arguments will always produce the same return value. The React Docs seem to indicate that they intend to treat these as pure functions similarly to the second option I described, though it does not yet do so.

I like your description of the approach. It also is nice and easy to explain - basically, we're saying that the following two things will be roughly equivalent:

function Foo({ a, b }) {
  return <div class="foo">{a * b}</div>;
}

class Foo {
  shouldComponentUpdate() {
    return false;
  }
  render({ a, b }) {
    return <div class="foo">{a * b}</div>;
  }
}

This is also neat, because that's how I've actually achieved the exact effect we're going for in existing Preact codebases:

// Turn a pure function into an optimized component
const pure = fun => class {
  shouldComponentUpdate(){ return false }
  render(props, state, context) { return fun(props, context) }
};

// usage:
export default pure( ({ a, b }) => (
  <div class="foo">{a * b}</div>
);

As for help, I'm happy to have either! I don't mind taking a stab at this as another 5.0.0 beta release, it just might take me a day or two. I'd love to have test feedback though, since that's my main concern here (breaking existing code).

@idchlife
Copy link
Author

Well I'm working now in company where frontend based fully on preact, also we will be using preact-router, I'm looking forward to test new features and write feedback. And yep, it's time to check insides of preact, since I basically never saw them :(

And I have working applications on preact, will update them and also send feedback.

About approach - yes, react intends to act also like this (stateless functions are won't be called until props change), but talks about implementation goes from 2015. Don't know why they didn't yet implement this.

@developit
Copy link
Member

Awesome! I'm always happy to have more eyes on the codebase. I try to hang out in the preact gitter if you run into things while diving in :)

@developit
Copy link
Member

@idchlife & @aeosynth: Just came across a reason why this might be slightly less straightforward than we thought: context. A "pure" function can still react to values from context since it's passed in as the second argument - and it'd still technically be pure.

That being said, perhaps it's just as simple as using both props and context as the cache key for functional components.

@aeosynth
Copy link
Contributor

imo do whatever's simplest, because you'll need to track facebook's decision for compatibility

@developit
Copy link
Member

Also I guess recompose has a simple high order function for this:

import pure from 'recompose/pure';
const Optimized = pure(Normal);

@slmgc
Copy link
Contributor

slmgc commented Jun 4, 2016

@idchlife @aeosynth @developit I could be wrong, but here's my two cents' worth:
I don't think that pure functional components should ever react to context changes. These components should be plain and simple, getting all what they need from container components which could pass down any context changes as props.

@developit
Copy link
Member

Been thinking about this and it seems you're right - originally I commented saying they would have to take context into account, but in reality they are far more likely to only pass context through the tree. Since this is automatic, it would still happen even if rendering a memoized input.

@idchlife
Copy link
Author

idchlife commented Jun 5, 2016

@slmgc @developit I vote for only props, but I'm not sure about react decision on this case: https://facebook.github.io/react/docs/context.html#referencing-context-in-stateless-functional-components

@developit diff of two object will cause bigger perfomance penalty comparing to only props?

I personally did not yet use context actually.

@developit
Copy link
Member

@idchlife meant to ping you about this earlier - are you using a state wrapper or redux or anything? That would already be giving you the prop based diff. Just curious!

@idchlife
Copy link
Author

@developit actually I'm using standard flux implementation as seen in Facebook examples. Been using it for a year now. Yet not redux/anything else. Tried, staid with Flux. Somehow, Flux with constants, actions, stores makes code really transparent and simple for me and my colleagues. So for state diff I must check it manually. You talking about this, yes? Emitting change on state modified?

@developit
Copy link
Member

Makes sense and good to know, we're doing something similar. This issue is next on my list :)

@developit
Copy link
Member

I might punt on this for the 5.x release because we don't really know what React is going to do yet.
For now, this is what I'd recommend:

/** Wrap a given (functional) component in a sCU:false HOC.
 *  @example
 *    const Foo = pure( ({ a, b }) => (
 *      <div>{a+b}</div>
 *    ));
 */
const pure = fn => class Pure extends Component
  shouldComponentUpdate(props) {
    for (let key in props) if (props[key]!==this.props[key]) return true;
    for (let key in this.props) if (!(key in props)) return true;
    return false;
  }
  render(props) {
    return h(fn, props);
  }
};

@idchlife
Copy link
Author

@developit right know thinking about doing this yes. Thank you for your work! Waiting for update ;)

@pl12133
Copy link
Contributor

pl12133 commented Jul 15, 2016

/** Wrap a given (functional) component in a sCU:false HOC.
 *  @example
 *    const Foo = pure( ({ a, b }) => (
 *      <div>{a+b}</div>
 *    ));
 */
const pure = fn => class Pure extends Component
  shouldComponentUpdate(props) {
    for (let key in props) if (props[key]!==this.props[key]) return true;
    for (let key in this.props) if (!(key in props)) return true;
    return false;
  }
  render(props) {
    return h(fn, props);
  }
};

Great idea @developit, it actually looks like this is on track to make it into React core: facebook/react#7195

EDIT: Fixed the quoted code.

@developit
Copy link
Member

@pl12133 now we have to make a call on whether PureComponent is a feature we want in Preact core, or if it makes its way into preact-compat.

@aeosynth
Copy link
Contributor

core please

@slmgc
Copy link
Contributor

slmgc commented Jul 15, 2016

@developit I think PureComponent should be in preact-compat like Component which is already there.

@idchlife
Copy link
Author

idchlife commented Jul 15, 2016

@developit I think we should wait and see.

So basically they will add Pure into the core and forget all the story about stateless pure function? Oh my god. There was such hype about stateless function component being without re-render if props are the same. There were talks about adding this into the react core.

From my point of view Pure should be not really in preact-compat but maybe something like preact-helpers or something like this (did not yet think about it, but many 'helpers' for development (warnings, errors, advices in console) and production 'optimizations' need to be somewhere around but not in core because well if you don't need it don't use it (since all those things will bloat final transpiled code))

Is there good example of core package and package added for development/production in some library? (like koajs and koa-middleware smthn) I mean that is also well contributed to and important as core.

@slmgc I think preact has it's own Component.

@slmgc
Copy link
Contributor

slmgc commented Jul 15, 2016

@idchlife, yeah I know that preact has it's own Component but it's been extended in preact-compat with additional properties which also makes sense with PureComponent.

@idchlife
Copy link
Author

@slmgc oh I see. Yes it really does! But there is difference between two cases.

React.Component is for compatibility, but PureComponent is just eh like some custom code, like which everyone could and did write on their own until now. I'm seriously confused that they are going to add this 'help class' into the core. This doesn't make much sense. Just Component extending component or made for functions.

@developit
Copy link
Member

developit commented Jul 15, 2016

I just took a second look at the issue @pl12133 pointed to, and I'm a little disappointed. If I recall, React team had originally promoted PureComponent as a sort-of "flag" that would trigger all descendant stateless pure functions to be also treated as pure (they are currently not). What is added in that PR is just a shouldComponentUpdate() optimization baked into core, as you guys noted.

I think there are a few chances here for Preact to be a little ahead of the game. Right now, Preact aggressively recycles (reclaims) stateful components. However, in order to maintain parity with React it discards the actual component instances and only reuses their DOM.

I think it might be worth investigating adding a flag that inverts this - that flag could even be a Preact version of PureComponent - there is nothing saying the internals of PureComponent have to match React's implementation, especially if we have the opportunity to improve on it.

So at baseline, I'd see a proper Preact core version of PureComponent doing:

  • shouldComponentUpdate():false optimization
  • Re-using instances across recycling (can just reset this.state or re-invoke getInitialState()?)
  • Entering into a "pure" mode for descendant stateless pure functions (up to the next inner Component instance).

I don't have a good idea how much weight we'd be looking at for this, though, and I still have fears around PureComponent being a magical solution that few understand but everyone wants to use. React has suffered from that in the past, we should try to avoid the same situation.

Thoughts?

-- edit -- by the way, just wanted to mention this issue is pretty stellar. it's so nice to have more people thinking through these decisions! 🍻

@slmgc
Copy link
Contributor

slmgc commented Jul 15, 2016

@developit does it make sense to end this "pure" mode upon entering the next inner Component instance? Because this instance won't get updates until the "pure" parent Component receives any.

@idchlife
Copy link
Author

@developit globally I would like to discuss the question - why everyone returned to PureComponent just being class with extended shouldComponentUpdate functionality instead of 'let all functions component be pure because there are no much purpose for them to exist without being pure!'.

@slmgc I'll give my 2 cents about this. I think pure component would prevent refreshing it's children because that's the way it should be in my opinion. I don't think we should make it this clever.

@developit
Copy link
Member

@idchlife True, we were originally looking into just making the call and saying "pure functions are pure", which honestly seems like a very acceptable tradeoff to make. Yes it's different from React, but that's basically the whole point - if we can make better decisions without baggage, we should.

Regarding @slmgc's point - it's an odd case, and I'm wondering if that's why the React guys shied away from the more involved descendant optimization logic.

Perhaps the road forward here is to relegate PureComponent (in terms of matching React's implementation) to preact-compat, but take the spirit of that into Preact core:

  • optimizing pure functions equivalent to shouldComponentUpdate():false
  • hyper-optimizing memoized pure functions (if it returns the same root VNode, no diff is performed)

My concern about potentially bringing the "simple" PureComponent into core would just be that it's not really a silver bullet of any kind, despite being named as such. As people mentioned on the React issue, it ends up doing nothing at all when there are event handlers or nested objects, which are a very common usages. Worse still, since PureComponent adds two shallowEqual() calls on every render, it would actually be quite a bit slower on valid renders.

@developit
Copy link
Member

FWIW, in a bunch of more recent testing I've been doing while making performance improvements in 5.x, the expense of memoizing and caching pure functions ends up actually making it slower overall than just letting Preact's diff handle everything. Still possibilities here, but I don't think we're seeing much impact by not having this yet.

@idchlife
Copy link
Author

@developit sorry from this offline from the issue.

You added something like this? (pseudocode)

if typeof comp is function:
  if newProps !== comp.oldProps:
    comp.render()

And thus 'newProps !== comp.oldProps' is slower than letting preact actually diff in render?

If that's the case then preact diff is literally jackpot here and no more code for diffing pure functions needed. Until new faster 'newProps !== comp.oldProps' solution found, of course.

@slmgc
Copy link
Contributor

slmgc commented Jul 24, 2016

@idchlife I think it was more like:

const keys = Object.keys(newProps)

if (
    keys.length !== Object.keys(oldProps).length ||
    keys.filter(key => !shallowEqual(newProps[key], oldProps[key])).length
) comp.render()

@developit
Copy link
Member

developit commented Jul 24, 2016

Correct, the implementation is a shallow compare, which is relatively costly. Still needs some investigation, but I was seeing much better framerates with 100% pure components than with memoized ones. sCU:false still wins overall though (in most cases).

(back on a computer): I believe the reason for this is that for a single component, Preact's diff is actually faster than !shallowEqual(a, b). However, for a full tree of components, a shallowEqual() that disables larger branches of the tree beats out the diff (shouldComponentUpdate() turns off recursive updates).

That being said, memoized pure functions (where the root VNode returned is === to the previous if the props don't change) combined with the same shouldComponentUpdate():false logic would be the best possible scenario.

Since preact has a hook for all created VNodes, we can do an interesting experiment. The following code installs the optimizations we talked about throughout the tree:

import { options } from 'preact';

class Wrapper extends Component {
  shouldComponentUpdate(props) {
    for (let key in props) if (props[key]!==this.props[key]) return true;
    for (let key in this.props) if (!(key in props)) return true;
    return false;
  }
  render(props, state, ctx) {
    return this.renderChild(props, ctx);
  }
}

// wrap a function in a sCU:false component
function wrap(fn) {
  class Wrap extends Wrapper {}
  Wrap.prototype.renderChild = fn;
  return Wrap;
}

// install a global vnode creation hook that wraps all pure functional components
let old = options.vnode;
options.vnode = vnode => {
  let c = vnode.nodeName;
  if (typeof c==='function' && !(c.prototype && c.prototype.render)) {
    vnode.nodeName = c.__wrap || (c.__wrap = wrap(c));
  }
  if (old) old(vnode);
};

@developit
Copy link
Member

Update: it looks like React's intentions with PureComponent are to eventually use it to back Stateless Functional Components. That might be a good solution for Preact, too.

Whether we expose a PureComponent or not is its own decision, but the idea of internally wrapping SFC's in a backing instance will put Preact in a good position to perform further optimizations for SFC's. Right now pure functional components in Preact are basically ephemeral, which is beneficial for memory usage, but makes re-rendering optimizations a little trickier.

Moving to a backing instance would also set things up so we're ready to parallel whatever the "next-gen" functional components are, which seems to be intended for a near future version of React (functional components with state? 🙅).

@developit
Copy link
Member

Fixed in 8.

marvinhagemeister added a commit that referenced this issue Mar 2, 2019
Add raw flag to npm dev task
marvinhagemeister added a commit that referenced this issue Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants