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

Boost layout performance #2015

Merged
merged 7 commits into from
Jun 19, 2020
Merged

Boost layout performance #2015

merged 7 commits into from
Jun 19, 2020

Conversation

nstepien
Copy link
Contributor

@nstepien nstepien commented Apr 22, 2020

Since we know all the widths/heights/positions of rows/cells, we can absolutely position the rows, and replace two padding divs with very dynamic heights, with one div with a more static height.
This improves layout perf from ~5ms to ~3.1ms on my machine. Might also improve more than layout rendering, not sure.

Before:

image

After:

image

@nstepien nstepien self-assigned this Apr 22, 2020
@nstepien
Copy link
Contributor Author

I've also fixed summary rows positioning so they're always at the bottom of the grid, even if we don't have enough rows to fill the available space.

Before:

image

After:

image

@nstepien nstepien marked this pull request as ready for review April 22, 2020 13:06
@nstepien nstepien requested a review from amanmahajan7 April 22, 2020 13:06
@@ -47,6 +48,7 @@ function Row<R, SR = unknown>({
onDragEnter={wrapEvent(handleDragEnter, onDragEnter)}
onDragOver={wrapEvent(handleDragOver, onDragOver)}
onDrop={wrapEvent(preventDefault, onDrop)}
style={{ top }}
Copy link
Contributor

@qili26 qili26 Apr 22, 2020

Choose a reason for hiding this comment

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

Just 1 concern here. Let's say if the user has their own RowRenderer and they forget to add this top to their wrapper. The whole layout would be messed up, right?

Copy link
Contributor Author

@nstepien nstepien Apr 22, 2020

Choose a reason for hiding this comment

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

Yup. If you pass top back to Row, or to your wrapper, you'll be fine though.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I don't know if this worth it given by 5ms to 3ms improvement. As it makes the RowRenderer error prone. And I bet people will create issues for this case when they use CustomRowRenderer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aren't custom row renderers already error-prone?

@amanmahajan7
Copy link
Contributor

@nstepien can you catchup?

@nstepien
Copy link
Contributor Author

@amanmahajan7 Done.

@amanmahajan7 amanmahajan7 merged commit 8da2ca5 into canary Jun 19, 2020
@amanmahajan7 amanmahajan7 deleted the layoutperf branch June 19, 2020 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants