Skip to content

[PERF] Set default bufferSize to 1 #693

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

Merged
merged 1 commit into from
Jun 3, 2019
Merged

[PERF] Set default bufferSize to 1 #693

merged 1 commit into from
Jun 3, 2019

Conversation

cyril-sf
Copy link
Contributor

@cyril-sf cyril-sf commented May 30, 2019

By default, ember-table was allocating a buffer of 20 rows before and after the visible ones.

This default value forces a lot of computation in the case of tables with many columns and complex cells, from syncinc more data (from rows to cells for example) and triggering more observers
(based on the number of computed properties defined per cell for example).

vertical-collection also does not allocate the part of the buffer that should be above the visible first row on the initial render, as there is none when loading the table. This means that the initial scroll will also lead to the creation of 20 rows in the DOM (and the underlying computation to render the content).

From a pure UI/UX point of view, we don't need more than 1 row on each side of the table,
and it's already the default for vertical-collection.

By default, ember-table allocates a buffer of 20 rows
before and after the visible ones.

This default value forces a lot of computation in the case of tables with many columns and complex cells,
from syncinc more data (from rows to cells for example) and triggering more observers
(based on the number of computed properties defines per cell for example).
VC also does not allocate the part of the buffer that should be above the visible first row,
as there is none when loading the table. This means that the initial scroll will also
lead to the creation of 20 rows in the DOM (and the underlying computation to render the content).

From a pure UI/UX point of view, we don't need more than 1 row on each side of the table,
and it's already the default for `vertical-collection`.
@cyril-sf cyril-sf changed the title [PERF] Set default bufferSize to 1 [PERF] Set default bufferSize to 1 on ET2 May 30, 2019
@cyril-sf cyril-sf changed the title [PERF] Set default bufferSize to 1 on ET2 [PERF] Set default bufferSize to 1 May 30, 2019
@mixonic
Copy link
Member

mixonic commented May 31, 2019

I think this is obviously correct for Addepar right now, I'm curious if there is anyone else out there for whom this is notable.

@cyril-sf
Copy link
Contributor Author

@mixonic do you have concerns about merging this PR?

@mixonic
Copy link
Member

mixonic commented May 31, 2019

@cyril-sf I don't think we understand why the value is explicitly 20. Perhaps @pzuraq can share thoughts.

This is also easy to override in a consuming app by passing an argument to ember-tbody

@cyril-sf
Copy link
Contributor Author

cyril-sf commented May 31, 2019

While it's easy, it also means that ember-table doesn't deliver necessarily good performance out of the box.

20 is an arbitrary number, there's no reason for it to be the correct answer to any bug IMO.

@rondale-sc
Copy link
Contributor

Yeah, I wonder why VC's default is 1 though? Seems strange, no?

@pzuraq
Copy link
Contributor

pzuraq commented May 31, 2019

I believe we made this change because we were seeing a lot of lag if users scrolled quickly. While more items do get rendered, and more are rendered on initial render, performance should be equivalent for subsequent rerenders. That said, I don't have a strong preference here one way or another.

Copy link
Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the change to 20 was landed as a performance tweak for a given case, I have no problem changing the default to match that of vertical collection.

@cyril-sf cyril-sf merged commit 6c1e84f into master Jun 3, 2019
@cyril-sf cyril-sf deleted the cyril/buffer-size branch June 3, 2019 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants