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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove component recycling #664

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@NekR
Collaborator

NekR commented Apr 23, 2017

I went ahead 馃槺

@NekR NekR requested a review from developit Apr 23, 2017

@NekR

This comment has been minimized.

Show comment
Hide comment
@NekR

NekR Apr 23, 2017

Collaborator

First hand info: this hurts performance.

Collaborator

NekR commented Apr 23, 2017

First hand info: this hurts performance.

@NekR

This comment has been minimized.

Show comment
Hide comment
@NekR

NekR Apr 23, 2017

Collaborator

What about shouldRecycle() { return false } life-cycle method?

Collaborator

NekR commented Apr 23, 2017

What about shouldRecycle() { return false } life-cycle method?

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Apr 25, 2017

Owner

Can get some perf back by removing this line - right now anything that was triggering a component recycle is discarding that component's DOM. With recycling removed, it can instead keep it and diff it. It's funny, that is actually how Preact worked prior to 4.x 馃槄

Owner

developit commented Apr 25, 2017

Can get some perf back by removing this line - right now anything that was triggering a component recycle is discarding that component's DOM. With recycling removed, it can instead keep it and diff it. It's funny, that is actually how Preact worked prior to 4.x 馃槄

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5d7f02e on NekR:v8-no-recycling into dee8c00 on developit:master.

coveralls commented Apr 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5d7f02e on NekR:v8-no-recycling into dee8c00 on developit:master.

@NekR

This comment has been minimized.

Show comment
Hide comment
@NekR

NekR Apr 25, 2017

Collaborator

@developit I'm still not sure about the right way here. I didn't test with your perf-test branch yet, but I'll eventually.

Collaborator

NekR commented Apr 25, 2017

@developit I'm still not sure about the right way here. I didn't test with your perf-test branch yet, but I'll eventually.

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Apr 26, 2017

Owner

All good - this is something we have to be super careful with, it'll take a while and some new tests to verify we're not regressing performance (or if we are, that it's controlled).

Owner

developit commented Apr 26, 2017

All good - this is something we have to be super careful with, it'll take a while and some new tests to verify we're not regressing performance (or if we are, that it's controlled).

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 26, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling fd53457 on NekR:v8-no-recycling into c4f3fb0 on developit:master.

coveralls commented Apr 26, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling fd53457 on NekR:v8-no-recycling into c4f3fb0 on developit:master.

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Jun 27, 2017

Owner

Leaving a note here for myself and others. Here's the work I did to adjust the rest of the diff so that it actually leverages the recycler removal for some performance gains:
NekR/nekr-preact@v8-no-recycling...developit:scratch/perf

Owner

developit commented Jun 27, 2017

Leaving a note here for myself and others. Here's the work I did to adjust the rest of the diff so that it actually leverages the recycler removal for some performance gains:
NekR/nekr-preact@v8-no-recycling...developit:scratch/perf

@developit developit added the important label Jun 27, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d4501ec on NekR:v8-no-recycling into d49ba50 on developit:master.

coveralls commented Jun 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d4501ec on NekR:v8-no-recycling into d49ba50 on developit:master.

@PepsRyuu

This comment has been minimized.

Show comment
Hide comment
@PepsRyuu

PepsRyuu Dec 14, 2017

Is there any update on this? Had an issue with Preact showing old images until the new src loaded, and had to resort to embedding Preact into the project with "collectComponent()" commented out to fix it. Would be great to have a temporary option to disable component recycling until the proper removal and performance tweaks are in place.

PepsRyuu commented Dec 14, 2017

Is there any update on this? Had an issue with Preact showing old images until the new src loaded, and had to resort to embedding Preact into the project with "collectComponent()" commented out to fix it. Would be great to have a temporary option to disable component recycling until the proper removal and performance tweaks are in place.

@kurtextrem

This comment has been minimized.

Show comment
Hide comment
@kurtextrem

kurtextrem Aug 12, 2018

This issue has been mentioned in the blog post about Uber Mobile: https://eng.uber.com/m-uber/. Just fyi.

kurtextrem commented Aug 12, 2018

This issue has been mentioned in the blog post about Uber Mobile: https://eng.uber.com/m-uber/. Just fyi.

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