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

perf(module:select): do not run change detection if the triggerWidth has not been changed #6858

Merged
merged 1 commit into from
Nov 4, 2021

Conversation

arturovt
Copy link
Member

PR Checklist

PR Type

[x] Refactoring (no functional changes, no api changes)

What is the current behavior?

Currently, the nz-select calls updateCdkConnectedOverlayStatus every time the select is opened or closed. The updateCdkConnectedOverlayStatus schedules a frame callback which is triggering the whole layout update (due to getBoundingClientRect()) and it calls markForCheck() which forces Angular to run the change detection from the root component down to nz-select.

It causes frame drops since the main thread is busy with:

  1. updating layout
  2. executing tick()

We don't need to run the markForCheck() since we don't need the "global" change detection. The triggerWidth is bound to cdkConnectedOverlayMinWidth, the triggerWidth is a number, number bindings are not updated each change detection cycle, since Angular uses Object.is comparison to determine if the binding has been changed or not:

function bindingUpdated(lView, bindingIndex, value) {
  const oldValue = lView[bindingIndex];
  if (Object.is(oldValue, value)) {
    return false;
  }
}

This means that if the triggerWidth hasn't been updated, the cdkConnectedOverlayMinWidth binding will not be re-evaluated. This means we have a "dead" change detection (the tick() has been run, but there's nothing to update).

image

What is the new behavior?

The change detection is not running anymore from the root component when the frame callback fires since it'll check if the triggerWidth has been changed or not.

Does this PR introduce a breaking change?

[x] No

@zorro-bot
Copy link

zorro-bot bot commented Jul 13, 2021

This preview will be available after the AzureCI is passed.

@codecov
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

Merging #6858 (1979f61) into master (b18c050) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 1979f61 differs from pull request most recent head a3deb0e. Consider uploading reports for the commit a3deb0e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6858      +/-   ##
==========================================
+ Coverage   89.52%   89.55%   +0.02%     
==========================================
  Files         489      489              
  Lines       15768    15810      +42     
  Branches     2573     2429     -144     
==========================================
+ Hits        14117    14158      +41     
+ Misses       1011     1008       -3     
- Partials      640      644       +4     
Impacted Files Coverage Δ
components/select/select.component.ts 87.42% <100.00%> (+0.24%) ⬆️
components/core/polyfill/request-animation.ts 28.57% <0.00%> (-4.77%) ⬇️
components/time-picker/time-holder.ts 94.73% <0.00%> (-2.11%) ⬇️
components/table/src/table-style.service.ts 94.82% <0.00%> (-1.73%) ⬇️
components/core/util/text-measure.ts 89.56% <0.00%> (-1.67%) ⬇️
components/code-editor/code-editor.component.ts 14.75% <0.00%> (-0.95%) ⬇️
...ponents/time-picker/time-picker-panel.component.ts 88.57% <0.00%> (-0.91%) ⬇️
components/table/src/cell/td-addon.component.ts 75.86% <0.00%> (-0.81%) ⬇️
components/modal/modal-container.ts 92.34% <0.00%> (-0.55%) ⬇️
components/core/tree/nz-tree-base.service.ts 81.75% <0.00%> (-0.54%) ⬇️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b18c050...a3deb0e. Read the comment docs.

Copy link
Member

@hsuanxyz hsuanxyz left a comment

Choose a reason for hiding this comment

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

LGTM

@vthinkxie vthinkxie merged commit 055f4a1 into NG-ZORRO:master Nov 4, 2021
@arturovt arturovt deleted the perf/select-trigger-width branch November 4, 2021 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants