Remove props-based render optimization

[mindplay-dk]

As concluded here, the recent optimization where rendering is skipped has caused me a lot of problems - more than a full day of debugging tests at this point.

I'd like to suggest we remove this optimization.

There's a reason why other engines don't use this type of optimization:

https://codesandbox.io/s/festive-chaplygin-vj7nd

As you can see, the <Clock/> component doesn't update - even though it's parent <App/> is updating. This is quite surprising, since updates are usually top-down in other engines - if the parent updates, as a rule, all of it's children are going to update.

Now, you and I can agree that a function ideally shouldn't depend on external state the way this does - but, in a language that doesn't support pure functions as a concept, this is normal and expected, and the consequence of an
optimization like this is confusing and unexpected.

Side-effects are just a part of the language that we have to deal with.

As we discussed earlier, this optimization has very little value to begin with, since anything like an array or event-handler will throw it off - those are common enough that this optimization isn't likely to help most of the time.

Instead, I'd like to suggest a safer approach.

We can already optimize rendering with useMemo:

function Component({ items, stuff, cake }) {
  return useMemo(() => {
    return (
      <div> ... big expensive tree ... </div>
    )
  }, [stuff, ...items])
}

This is easy enough to implement and gives much better control - in this example, cake doesn't affect the view, and all items do affect the view, so you have proper control of which values affect the view, can unpack arrays for comparison, and so on.

This approach also works better when you don't have side-effects - so if you take my example from before and move the time value out of the <Clock/> component where it should be, you can still optimize like this:

https://codesandbox.io/s/practical-grothendieck-u4sbv

This is a much better optimization overall, because the built-in optimization only bypasses reconciliation - the entire tree of nodes will still be calculated, so if the components in that tree are slow, it doesn't help at all, they will still take as much time as they need to calculate.

And finally, I wouldn't be surprised if the the current built-in optimization actually makes rendering slower overall. It has to compare individual properties of every component at every render, which is overhead, even in the cases where those props contain an array or event-handler - which in a real-world app is very likely to be most of the time.

Now, I can't actually tell if fre already optimizes for useMemo? But it should, and this is a much simpler, safer and faster optimization than comparing props: you can simply compare the entire oldNode === newNode by reference (same object) and in that case, you can safely bypass rendering of that node, since the user has chosen this optimization.

(That optimization also works for static trees, e.g. using a const declared outside of a function - if you have a footer or something that never changes, you can optimize by rendering the whole thing at startup and this never needs to update again.)

I think this optimization has caused enough problems already - even if I can fix all the test-cases, they will and up containing a lot of "nonsense" properties, which will make the tests harder for someone to understand.

But mostly, I worry that users will likely run into similar problems in real projects.

[yisar]

Thank you for your advice. I will explan this optimization later.

The reason you have problems is that you are used to the top-down rendering of react

But In Vue or any other framworks, the component is exact-updating, you can see follows:

vue exact updating

omi path exact updating

In our country China, most develops think Vue's approach is correct. I implemented this optimization in fre without dependces on collection. This is the biggest difference between fre and react so far, and there will be more in the future

Let me explain that this optimization mainly includes two aspects

1. Shallow comparison props

<Compoent /> // ×
<Compoent value={[]}/> // √

2. === comparison state

const [content, setContent] = useState('111')
setConetent('111') // × because state have not changed
setContent('222') // √

And finally, I wouldn't be surprised if the the current built-in optimization actually makes rendering slower overall.

It will be faster. It can skip reconciliation, but more importantly, it can skip commitWork and operate DOM, which is the worst performance.

But mostly, I worry that users will likely run into similar problems in real projects.

It has not many rules, but it is different from react, Most developers can adapt to it.

We can already optimize rendering with useMemo:

Yes, it may be OK, more like react.memo, but it just returns a cache value, it can not skip reconciliation and commitwork, which is meaningless.

It is very important that we skip the rendering of subcomponents is not to improve the performance. The main purpose is to skip the repeated initialization of hooks. This must prevent the execution of component instances within the framework core.

To sum up, I don't want to indulge react

I have to make different choices so that I can get rid of react and make breakthroughs.

Thank you for listening

[mindplay-dk]

To sum up, I don't want to indulge react

I have to make different choices so that I can get rid of react and make breakthroughs.

This isn't about being similar to React.

I don't think this optimization is a "breakthrough" - most of the time, this optimization won't apply because arrays or event-handlers change. When it does apply, so far it only seems to create problems.

Having confusing random properties in the tests is one thing (which will make it very difficult for contributors; in my case, already has) but I'm sure developers trying to adopt the framework will run into these problems in practice.

You can write down rules, but many developers won't read them, won't understand what "pure function" or "side-effects" means, and so on. They will perceive this kind of behavior as bugs. Having rules for hooks is bad enough - having even more rules will make it very frustrating for new users.

Also, as said, I'm not convinced this is an optimization in the first place, since this makes all updates slower, in favor of making a few updates faster - if this optimization makes any practical difference at all, it's likely to cancel itself out with all the extra comparisons it has to do.

I don't think you have a benchmark? So you probably don't know if this is faster or slower. It's, either way, a good idea to wait with this kind of systemic optimizations until you have a working test-suite and a reliable benchmark.

In my opinion, this has already created more problems than it solves. 😕

It is very important that we skip the rendering of subcomponents is not to improve the performance. The main purpose is to skip the repeated initialization of hooks. This must prevent the execution of component instances within the framework core.

Yes, that is one problem with hooks - and useMemo doesn't solve this, I understand... This comes back to the problem with not having a distinct initialization phase (as discussed in #106) since all components need to render at least once, and there's no distinction between initialization and updates...

You can already solve that using a higher-order function though:

function FastComponent(props) {
  return useMemo(
    () => <SlowComponent {...props}/>,
    [props.a, props.b]
  )
}

function SlowComponent(props) {
  useState(...)
  useState(...)
  useState(...)
  useEffect(...)
  useEffect(...)
  useEffect(...)

  return <div> ... </div>
}

(Turning FastComponent into a general higher-order function, rather than writing one for every component, should be easy enough...)

Could we think of another way to solve this without useMemo?

Maybe some sort of hook that lets you control whether or not a component should update? There's no real equivalent to shouldComponentUpdate with hooks, perhaps it would help to add one?

const Clock = ({ count, time }) => {
  useDeps(props => [props.count])

  return (
    <div>The time is: {time} ({count} updates)</div>
  )
}

So this new hook useDeps would let you manually specify the dependencies you want to check before performing an update - the framework would call this function and compare the return values, same way it currently compares props.

This could accept empty argument as well - so useDeps() would default to comparing all props, but that way, this optimization is opt-in ... no surprises by default.

Just shooting out ideas here. In my opinion, anything that's explicit but easy to apply is better than something implicit and surprising that you can only opt-out of with a hack.

[yisar]

Maybe some sort of hook that lets you control whether or not a component should update? There's no real equivalent to shouldComponentUpdate with hooks, perhaps it would help to add one?

Unfortunately, there is no such thing in theory, because the implement of hooks is after the instantiation of components.

In my opinion, this has already created more problems than it solves.

Yes, it creates more constraints and problems than we solve.

I don't think you have a benchmark?

Yes, there is no benchmark here, but you know, react also has no benchmark. They always say that the execution of JS is not worth money, but there is no benchmark at all.

https://codesandbox.io/s/practical-grothendieck-u4sbv

The child component should not rerender, because the state is not changed, the update should only cause by setState itself, UI = F(state)

In fact, I've always wanted to say that the performance and the problems are not the focus. The focus is that I don't think the top-down rerendering in React is right. This kind of thinking is wrong in itself. The view update should only be triggered by setState（F）. If the state or props do not changed, the component should not be rerender.

I don't know why you think react is right, but I want to make diffence with react, not keep the same rendering behavior as it

This may cause more problems. We can fix them, not remove exact updating.

[yisar]

I've been study with this for a long time. I want to show you a common demo.
You can see the console and react has a lot of repeated rendering, which has nothing to do with performance. Even if the performance is worse, it should not be repeated rendering.

https://github.com/yisar/fre/blob/master/demo/src/with-context.js

[yisar]

I think it's a different render way, not only an optimization. It's hard for us to convince each other 😭

[mindplay-dk]

I couldn't understand why this would cause more than one render, so I dropped your example into a sandbox for comparison between react, preact and fre.

https://codesandbox.io/s/sleepy-satoshi-dc4ej

I can see how fre is getting the minimum number of updates:

render App 
render A 
toggle theme 
render App 
render A 

Compared with react doing a crazy amount of extra updates:

render App 
render A 
toggle theme 
render App 
render A 
render App 
render A 
render A 

Compared with preact, which does only one unnecessary render of A:

render App 
render A 
toggle theme 
render App 
render A 
render A 

So preact is more optimized than react in this way, and fre obviously gets this perfect.

What I don't understand is why would any of them be doing any extra updates?

The whole point of having a render queue is to eliminate unnecessary updates. In your example, all the updates are schedule synchronously, as far as I can tell, so the render queue should have all the information necessary to discover that A, being located in the subtree of App, will already be covered by an update of App, and should be able to eliminate that extra update from the queue.

At least preact is very close to getting this right, I don't know why it doesn't get this perfect.

But my point here, is it accomplishes this without comparing properties, and I still don't understand why that would be a better solution - this must be slower and more complex than simply cleaning duplicate entries from a render-queue before starting the updates?

Otherwise, what's the point of having a render queue in the first place? Frameworks like preact, which don't have scheduling, could just perform all updates synchronously as they happen - which would give a result much like react, I suppose. The queue is there to solve that - if it worked better, surely there wouldn't be any need to compare properties or break expectations about side-effects...? 🤔

[yisar]

Let me explain the difference between fre, preact and react
Fre, props and state need to be compared
Preact, compare state only, if oldState === newState，the component will not rerender
React, nothing to compare. the component will rerender every time.

In addition, preact and fre have a similar update queue. If the parent component is already in the queue, its children will not be pushed into the queue.

[mindplay-dk]

What I'm trying to explain is actually what I was expecting when I wrote the "async state update" test - remember this one?

test('async state update', withDOM(async is => {
  let updates = 0

  const Component = () => {
    const [count, setState] = useState(0)

    updates += 1

    return <button onClick={() => setState(count => count + 1)}>{count}</button>
  }

  await testUpdates([
    {
      content: <Component/>,
      test: ([button]) => {
        is.equal(+button.textContent, 0)
        is.equal(updates, 1)

        // trigger several functional state updates:
        button.click()
        button.click()
        button.click()
      }
    },
    {
      content: <Component/>,
      test: ([button]) => {
        is.equal(+button.textContent, 3) // all 3 state updates applied
        is.equal(updates, 2)             // <-- THIS ONE
      }
    }
  ])
}))

This test actually passes now - and notice how content: <Component/> does not use the array hack here to force and update. The point of this test was to demonstrate we're not doing unnecessary updates.

When I first wrote this test, I was expecting only 2 updates in that last assertion - the 3 simulated clicks, and the subsequent update of the parent component inside testUpdates, all get scheduled synchronously before the scheduler gets to process the requested updates.

So I think we were expecting the same result for two different reasons?

I was expecting this test would pass because the queue would recognize that all of these render requests are trying to update the same component - this is how I understood at least preact to work.

In addition, preact and fre have a similar update queue. If the parent component is already in the queue, its children will not be pushed into the queue.

Then I don't understand why your example would cause any extra updates? If you were to remove the props comparison optimization, you should get the same number of updates??

[yisar]

So I think we were expecting the same result for two different reasons?

Yes, this test case is correct, because in any case, as long as this component triggers setState (button. Click ()), this component will be updated.

Then I don't understand why your example would cause any extra updates?

This demo is a simple pub-sub. In fact, when we use react, there are many duplicate updates, but the react team doesn't care and developers don't notice either

If you were to remove the props comparison optimization, you should get the same number of updates??

Maybe worse, preact maybe has other optimizations, fre has nothing else

.

[hkc452]

I can't help saying that it's very easy to break the so call optimization when it comes to props.children and do not work out as it blows, (: escape

[yisar]

it's very easy to break the so call optimization.

Even if the optimization is broken, the rendering result is correct. React is only the worst.

[hkc452]

it's very easy to break the so call optimization.

Even if the optimization is broken, the rendering result is correct. React is only the worst.

not exactly, that at some point, it maybe can reduce the render times or act just like react , but not that accuratly update like vue

[yisar]

but not that accuratly update like vue.

Vue is not absolutely accurate, it is also limited by props-comparison rules, but I admit that fre optimization is more likely to be broken.

This is because most of the users of fre come from react. They are not noticing props comparison.

The above discussion is a good summary, this optimization may be wrong, let time tell us, maybe I will reevaluate.

[mindplay-dk]

My biggest concern here is, you seem to be prioritizing theoretical performance gains over familiar developer ergonomics.

This optimization wouldn't work for most of the components in most of my projects.

It also wouldn't work in projects that use functional state managers such as Redux, since all objects and values are new copies with every update - and, typically, projects with functional state managers are going to need useMemo under any circumstances to make sure they render only when certain states change, and for those cases you have the object identity optimization for VNodes.

If this optimization was more reliable, I might have a different opinion - but if you were to improve it by adding something like fast-deep-equal for comparison, that's going to add even more overhead when comparing props, and it still doesn't solve the problem with event handlers.

I am curious to know how many updates your example would do without the props comparison optimization - if the render queue itself is doing a good job removing duplicate updates (as good or better than preact) then it doesn't seem like a big problem. Even if it's not theoretically perfect, react is doing the worst job of all, and still it's the most popular framework out there.

Developers tend to pick things that work for them and not worry much about performance until it becomes a problem - which, most of the time, it doesn't, not even with the terrible update scheduling in react ... and when it does, there are easy ways to optimize, like useMemo.

I'm sorry, I'll shut up now. 😄

[yisar]

To be honest, at present, I don't care much about its performance. I just want to have a mechanism. This mechanism can give us more opportunities to arrange updates, when component should be updated and when it should not. This is a new direction.

It's true that it has some limitations and problems now. Do we have any way to solve them?
In fact, I'm not entirely sure it's right or not. Maybe I will remove it or have a better solution in the future.

Let's keep looking forward 👀

[yisar]

I'm back! I decide to adopt this issue and delete it in fre2.

The reason is that I found a better way to implement composition API. The new implementation does not need to change the internal source code of the framework. Its intrusiveness is zero 0️⃣.

Here: https://github.com/yisar/qox

[mindplay-dk]

Looks interesting! 🙂

Needs more documentation/examples though? The site you link to in the description isn't working. (yet?)

[yisar]

Needs more documentation/examples though? The site you link to in the description isn't working. (yet?)

I want to use markdown and gitbook to generate documents, but unfortunately, gitbook is also in the blacklist of Chinese Great Firewall, and I can't access it 😢

[mindplay-dk]

MkDocs might be worth a look? You can self-host or build flat HTML files to deploy on a GitHub site.

[yisar]

#134
Now Fre.memo is ready, I will close this issue first 💯