Rendering performance improvement for reports with many rows #18405
Conversation
bx80
added
not-in-changelog
There was a problem hiding this comment.
This one can't be simply removed. The row highlight is covered by CSS. But when you hover a column, that column receives a class in each row. I don't think this can be covered by CSS. Now the ratio values aren't displayed (e.g. for visits or page views) anymore as it relies on that class. See
matomo/plugins/CoreHome/stylesheets/dataTable/_dataTable.less
Lines 36 to 38 in 8066b35
|
The 'give all values consistent width' part of handleColumnHighlighting() is the real performance hit, adding mouse event handlers to add/remove classes doesn't really impact loading speed. So I've restored the method and tested again with just the width setting code disabled and performance is basically the same as with the handleColumnHighlighting() method disabled. |
|
@sgiehl Following the theme of naively deleting random datatable functions... 😉 I think the performance heavy setFixWidthToMakeEllipsisWork() method can be replaced with a small CSS tweak. Setting the table cell width to calc(80%) should allow the ellipsis to be applied to the table label column using pure CSS without needing javascript to calculate and set a fixed cell width both initially and every time the table is resized. I've committed this change for review and it seems to work well in the tests I've done. Can you think of any problems with this approach or anything I should specifically test? |
There was a problem hiding this comment.
Oh dear. I would really love to get rid of that method. Struggling with it already for a while in #18386. The "problem" with all those methods is, that they are doing a lot more, than the name would let you think. The method setFixWidthToMakeEllipsisWork is actually also used to limit the width of a datatable to 1200px (if it's not widgetized), but it also ensures that a datatable doesn't always take all the space that would be available, but only what it really needs. In addition if fixes the size of the tables in "reportByDimension" view, which can be seen in the goal reports (the one with the additional dimension menu on the left). And I'm pretty sure it's responsible for some more stuff I don't see right now.
Removing that method actually broke most of the additional stuff...
Maybe for now remove that again, so we can already merge the other improvement. If needed you can then create new PRs with further improvements.
There was a problem hiding this comment.
The code changes look fine. But there are actually a lot failures in the UI tests. Once the new build is finished you may need to look through all those changes, and check if they are actually expected, or if there are some regressions, where the table sizing maybe doesn't work correctly anymore.
…tomo-org/matomo into m-18394-reports-performance-many-rows
|
I've updated all the screenshots for UI tests that failed from this change. |
There was a problem hiding this comment.
Looks got to merge now.
Btw. you forgot to create PR in submodule plugin to update the screenshots. I've done that for you. Also changing any submodule to another branch than 4.x-dev always needs to be reverted before a PR is merged. I'll do that once I saw if all related screenshot tests are passing.
Description:
refs #18394
Removes the obsolete handleColumnHighlighting() method which causes a significant performance hit when rendering a datatable with many rows.
Column width and row highlighting is now handled by CSS so this method should no longer be needed.
Review