Improve datagrid performance by martinRenou · Pull Request #394 · jupyterlab/lumino

martinRenou · 2022-09-07T10:42:46Z

cc. @ibdafna

This PR improves the performance of the grid by reworking the drawing of merged cells logic.

Before (resizing/scrolling a 1M cells grid with merged cells freezes):

before.mp4

After (resizing/scrolling a 1M cells grid with merged cells works smoothly):

after.mp4

Note for the tester: You will see a big difference in performance if you zoom out a lot and show a lot of cells.

afshin · 2022-09-07T12:33:56Z


    // Test whether there is content to blit.
-    let needBlit = curH > 0 && curH > 0 && width > 0 && height > 0;
+    let needBlit = curW > 0 && curH > 0 && width > 0 && height > 0;


🕵️ 🔍 🐜

resize(Column|Row|ColumnHeader|RowHeader)

martinRenou · 2022-09-07T14:19:33Z

This is the diff for the API:

diff --git a/review/api/datagrid.api.md b/review/api/datagrid.api.md
index 9197a504..ebd1c1c3 100644
--- a/review/api/datagrid.api.md
+++ b/review/api/datagrid.api.md
@@ -131,16 +131,11 @@ export interface CellGroup {
 export namespace CellGroup {
     export function areCellGroupsIntersecting(group1: CellGroup, group2: CellGroup): boolean;
     export function areCellGroupsIntersectingAtAxis(group1: CellGroup, group2: CellGroup, axis: 'row' | 'column'): boolean;
-    // (undocumented)
-    export function areCellsMerged(dataModel: DataModel, rgn: DataModel.CellRegion, cell1: number[], cell2: number[]): boolean;
-    export function calculateMergeOffsets(dataModel: DataModel, regions: DataModel.CellRegion[], axis: 'row' | 'column', sectionList: SectionList, index: number): [number, number, CellGroup];
     export function getCellGroupsAtColumn(dataModel: DataModel, rgn: DataModel.CellRegion, column: number): CellGroup[];
     export function getCellGroupsAtRegion(dataModel: DataModel, rgn: DataModel.CellRegion): CellGroup[];
     export function getCellGroupsAtRow(dataModel: DataModel, rgn: DataModel.CellRegion, row: number): CellGroup[];
     export function getGroup(dataModel: DataModel, rgn: DataModel.CellRegion, row: number, column: number): CellGroup | null;
     export function getGroupIndex(dataModel: DataModel, rgn: DataModel.CellRegion, row: number, column: number): number;
-    export function isCellGroupAbove(group1: CellGroup, group2: CellGroup): boolean;
-    export function isCellGroupBelow(group1: CellGroup, group2: CellGroup): boolean;
     export function joinCellGroups(groups: CellGroup[]): CellGroup;
     export function joinCellGroupsIntersectingAtAxis(dataModel: DataModel, regions: DataModel.CellRegion[], axis: 'row' | 'column', group: CellGroup): CellGroup;
     export function joinCellGroupWithMergedCellGroups(dataModel: DataModel, group: CellGroup, region: DataModel.CellRegion): CellGroup;

@afshin @blink1073 do you think that's a problem? I can add those back and add a DeprecationWarning if that helps the PR get merged.

martinRenou · 2022-09-07T14:23:55Z

A bi-product of this PR is that it fixes a rendering issue when shrinking cells that are at the same level as a merged cell:

Before:

before_issue.mp4

After:

after_issue.mp4

blink1073 · 2022-09-07T14:25:58Z

Removing them seems reasonable to me since datagrid is still pre-1.0.

martinRenou · 2022-09-07T14:56:26Z

Removing them seems reasonable to me since datagrid is still pre-1.0.

Thanks for your answer! I updated the API file

afshin

Nice work tracking down the performance issue and resolving it, @martinRenou!

I think we can merge this and iterate because:

This is a pre-1.0 package (although, perhaps we should consider jumping straight to 2.0 to be in line with the rest of the library)
@lumino/datagrid is downstream of the other packages

But we should make sure that before the final version we address:

refactoring with a type PaintRegion
implementing and removing the TODO messages
potentially de-duplicating the slightly different but very similar loops

martinRenou · 2022-09-09T06:59:45Z

But we should make sure that before the final version we address:

refactoring with a type PaintRegion

implementing and removing the TODO messages

potentially de-duplicating the slightly different but very similar loops

Definitely 👍🏽

Maybe @ibdafna would like to give this a try before we merge? As he implemented the merge cells logic and knows what to test etc to make sure everything works nicely.

ibdafna · 2022-09-09T07:05:28Z

Thank you for this @martinRenou, I will test tomorrow evening and report back. I have no doubt this is working incredibly well! 🙏

…

On Fri, Sep 9, 2022, 08:59 martinRenou ***@***.***> wrote: But we should make sure that before the final version we address: - refactoring with a type PaintRegion - implementing and removing the TODO messages - potentially de-duplicating the slightly different but very similar loops Definitely 👍🏽 Maybe @ibdafna <https://github.com/ibdafna> would like to give this a try before we merge? As he implemented the merge cells logic and knows what to test etc to make sure everything works nicely. — Reply to this email directly, view it on GitHub <#394 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFZICWMZELDW4KKGRPZURADV5LN6ZANCNFSM6AAAAAAQGUO24U> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ibdafna · 2022-09-11T15:25:24Z

127.0.0.1_8080.and.1.more.page.-.Personal.-.Microsoft.Edge.2022-09-11.08-23-26.mp4

@martinRenou I see some issues with border rendering when scrolling x/y axes (video attached)

martinRenou · 2022-09-12T08:07:52Z

Thanks for trying! I haven't seen those. I will try to replicate.

martinRenou · 2022-09-14T13:08:21Z

I can indeed replicate in the base example (the issue was not appearing in my custom grid for some reason).

I will investigate :)

martinRenou · 2022-09-19T14:57:45Z

@ibdafna the latest commit fc7acd4 is fixing the issue you mention above :)

scroll.mp4

EDIT: Looking at my screencast I now see an issue with the column header C 0, 4 which is not visible (I can reproduce the issue and working on it)

martinRenou · 2022-09-19T15:12:21Z

Looking at my screencast I now see an issue with the column header C 0, 4 which is not visible (I can reproduce the issue and working on it)

This is fixed by 32952d7. The issue was that I was taking into account the x-scroll level for the column header and the y-scroll level for the row header while this should not be the case (headers are sticky, not following the scroll).

martinRenou · 2022-09-19T15:47:06Z

@afshin @ibdafna this is ready for another review :)

ibdafna · 2022-09-19T17:20:11Z

LGTM! Let's ship it!! ♥💘💖💗💓💙💚💛❤🖤🧡💜

It appears that when slowly scrolling merged cells, the area considered "clean" is actually not really clean if a merged group is intersected by that area. Redrawing those groups fixes it.

martinRenou · 2022-09-20T08:45:55Z

Thanks!! I actually found another issue when slowly scrolling. 830e094 seems to fix that issue.

potentially de-duplicating the slightly different but very similar loops

The improvement suggested by @afshin gets more urgent to fix as it's now 6 loops that are quite similar, but I can do it in a separate PR if that's fine.

ibdafna · 2022-09-21T17:24:49Z

I'd love to see this released this week 😻

afshin

Thank you! 🚀

Explore datagrid performance improvements

2f4feb9

github-actions Bot assigned martinRenou Sep 7, 2022

martinRenou added the performance Addresses performance label Sep 7, 2022

blink1073 added the enhancement New feature or request label Sep 7, 2022

afshin changed the title ~~Improve datagrid performances~~ Improve datagrid performance Sep 7, 2022

afshin reviewed Sep 7, 2022

View reviewed changes

martinRenou added 13 commits September 7, 2022 16:16

Iterate

f5010d8

Iterate

e134881

Remove commented code and unused functions

bf6904a

Iterate

7801a85

Iterate

de11b2b

Remove merged cells handling in linegrid drawing

ce7e13f

Finally remove "shouldPaintEverything"

55a0431

Implement drawing borders

5142c92

Refactor drawing merged cells into one method

671d369

Fix not drawing borders outside of the paintRegion

c6ecd89

Fix colors

b85fdaa

Implement drawing merged cells for

073dee1

resize(Column|Row|ColumnHeader|RowHeader)

Linter

31de83b

martinRenou force-pushed the explore_datagrid_perf branch from 6c4f9ae to 31de83b Compare September 7, 2022 14:16

Update API file

03cde66

martinRenou marked this pull request as ready for review September 7, 2022 15:04

afshin approved these changes Sep 9, 2022

View reviewed changes

martinRenou added 3 commits September 19, 2022 11:40

Missing pixel

bc27956

More optimization

64312ce

Simplify code

fc7acd4

martinRenou added 2 commits September 19, 2022 17:02

Linter

e0fd7af

Fix headers positioning

32952d7

martinRenou added 2 commits September 19, 2022 17:42

Introduce a PixelRegion type

e8caef5

Linters

68e2223

martinRenou requested a review from afshin September 19, 2022 15:44

Fix issue when "slowly" scrolling merged cells

830e094

It appears that when slowly scrolling merged cells, the area considered "clean" is actually not really clean if a merged group is intersected by that area. Redrawing those groups fixes it.

martinRenou added 2 commits September 20, 2022 10:45

Linters

dbe9ce9

Battle testing

479075b

afshin approved these changes Sep 22, 2022

View reviewed changes

afshin merged commit ebaefcc into jupyterlab:main Sep 22, 2022

martinRenou deleted the explore_datagrid_perf branch September 23, 2022 07:27

github-actions Bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Uh oh!

Conversation

martinRenou commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afshin Sep 7, 2022

Choose a reason for hiding this comment

🕵️ 🔍 🐜

Uh oh!

martinRenou commented Sep 7, 2022

Uh oh!

martinRenou commented Sep 7, 2022

Uh oh!

blink1073 commented Sep 7, 2022

Uh oh!

martinRenou commented Sep 7, 2022

Uh oh!

afshin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinRenou commented Sep 9, 2022

Uh oh!

ibdafna commented Sep 9, 2022 via email

Uh oh!

ibdafna commented Sep 11, 2022

Uh oh!

martinRenou commented Sep 12, 2022

Uh oh!

martinRenou commented Sep 14, 2022

Uh oh!

martinRenou commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinRenou commented Sep 19, 2022

Uh oh!

martinRenou commented Sep 19, 2022

Uh oh!

ibdafna commented Sep 19, 2022

Uh oh!

martinRenou commented Sep 20, 2022

Uh oh!

ibdafna commented Sep 21, 2022

Uh oh!

afshin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

martinRenou commented Sep 7, 2022 •

edited

Loading

`🕵️ 🔍 🐜`

afshin left a comment •

edited

Loading

martinRenou commented Sep 19, 2022 •

edited

Loading