Skip to content

Conversation

@kunaljaykam
Copy link

  1. Fixes a gap from PR feat: renderer performance improvements #4134: Separates rendered() callbacks from height calculations
  2. Follows existing patterns: Matches the _quickNormalizeRowHeight() approach (lines
    632-640)
  3. Has precedent: Similar fix was already done for horizontal rendering (commit beff450)
  4. Clear performance benefit: Reduces layout thrashing during initial render

@rathboma rathboma requested a review from azmy60 February 5, 2026 13:45
Copy link
Collaborator

@azmy60 azmy60 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think this could be simpler by avoiding the extra loop. For example:

const rowsNeedingHeightInit = [];
renderedRows.forEach((row) => {
    row.rendered();

    if(!row.heightInitialized) {
        row.calcHeight(true);
        rowsNeedingHeightInit.push(row);
    }
});

rowsNeedingHeightInit.forEach((row) => {
    row.setCellHeight();
});

@kunaljaykam
Copy link
Author

thank you @azmy60 for the review. i agree but that would save one boolean check per row, but the performance win separating dom writes from dom reads will go away. rendered() triggers dom writes and calcHeight() reads layout properties combining them in one loop forces a browser reflow on every row. keeping them separate batches all writes first, then all reads, so the browser only reflows once.

Copy link
Collaborator

@azmy60 azmy60 left a comment

Choose a reason for hiding this comment

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

@kunaljaykam Ah I see. I wasn't aware of layout thrashing. Great catch!

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.

2 participants