perf(ivy): apply [style]/[class] bindings directly to style/className #33336

matsko · 2019-10-22T23:02:18Z

This patch ensures that the [style] and [class] based bindings
are directly applied to an element's style and className attributes.

This patch optimizes the algorithm so that it...

Doesn't construct an update an instance of StylingMapArray for
[style] and [class] bindings
Doesn't apply [style] and [class] based entries using
classList and style (direct attributes are used instead)
Doesn't split or iterate over all string-based tokens in a
string value obtained from a [class] binding.

This patch speeds up the following cases:

<div [class]> and <div class="..." [class]>
<div [style]> and <div style="..." [style]>

The overall speed increase is by over 5x.

packages/core/src/render3/styling/bindings.ts

matsko · 2019-10-23T01:15:41Z

presubmit = https://test.corp.google.com/ui#id=OCL:276185331:BASE:276185353:1571793286075:c4b0eb86

ivy presubmit = http://test/OCL:276185331:BASE:276185451:1571793334829:9fbcb88d

packages/core/src/render3/instructions/styling.ts

packages/core/src/render3/styling/bindings.ts

mhevery · 2019-10-23T01:51:12Z

packages/core/src/render3/styling/bindings.ts

+        valueToApply += forceStylesAsString(value as{[key: string]: any});
+        setStyleAttr(renderer, element, valueToApply);
+      }
+      return true;


Instead of return true convert this into if () { } else {} block.

Then extract the else block into a separate function. This makes applyStylingMapDirectly smaller and more inlinable (this improves performance about 10%).

packages/core/src/render3/instructions/styling.ts

IgorMinar · 2019-10-23T13:54:22Z

https://test.corp.google.com/ui#id=OCL:276265763:BASE:276265775:1571838704715:12c9aa66

IgorMinar · 2019-10-23T15:27:06Z

guitar presubmits are failing with some styling errors: http://fusion/2cb9bb68-1356-421b-8d6c-550c700bb15b

mhevery

LGTM

Let's land this PR as is.
I think there are a lot of micro-improvements we could do. Let's do that once we have time.

matsko · 2019-10-24T04:52:32Z

Presubmit = https://test.corp.google.com/ui#id=OCL:276415302:BASE:276415338:1571892303053:4a75eb1d

Ivy Presubmit = https://test.corp.google.com/ui#id=OCL:276415302:BASE:276415359:1571892318577:b924f16a

matsko · 2019-10-24T05:36:27Z

Presubmit = http://test/OCL:276421340:BASE:276421364:1571895364531:45bc88e0

Ivy Presubmit = http://test/OCL:276421340:BASE:276421383:1571895376432:d3d6fb19

matsko · 2019-10-24T19:12:13Z

Presubmit = http://test/OCL:276538658:BASE:276538713:1571944114374:d3b4df96

mhevery · 2019-10-24T19:07:10Z

packages/core/src/render3/instructions/styling.ts

+  // className / style value must be compared against.
+  const increment = isMapBased ? 2 : 1;
+  const index = lView[BINDING_INDEX] += increment;
+  return index - increment;


Could you rewrite this in a way which does not require both adding and subtracting the increment

mhevery · 2019-10-24T19:11:27Z

packages/core/src/render3/styling/bindings.ts

+  if (cachedValue === VALUE_IS_EXTERNALLY_MODIFIED) return true;
+
+  // web workers are not covered in this case
+  if (typeof Element === 'undefined' || !(element instanceof Element)) return false;


can you pull this out of this function. No need to execute `typeof Element === 'undefined' all the time.

const NO_ELEMENT = typeof Element === 'undefined' ;

pkozlowski-opensource · 2019-10-24T20:19:30Z

packages/core/src/render3/styling/bindings.ts

+
+  // comparing the DOM value against the cached value is the best way to
+  // see if something has changed.
+  const e = element as HTMLElement;


You could type the argument of this function as HTMLElement element (since you are already casting it to HTMLElement at the invocation place.

pkozlowski-opensource · 2019-10-24T20:24:25Z

packages/core/src/render3/styling/bindings.ts

+  // comparing the DOM value against the cached value is the best way to
+  // see if something has changed.
+  const e = element as HTMLElement;
+  const currentValue = isClassBased ? e.className : (e.style && e.style.cssText);


So, while I'm curious how it is going to perform, I'm also curious on how it is going to play with other renderers (ex. NativeScript). Other renderers might have a concept of a "class" and "style" but not necessarily store it at the className / style property.

As the very minimum we should probably guard it with a check for the existence of this property...

matsko · 2019-10-24T22:21:46Z

presubmit = https://test.corp.google.com/ui#id=OCL:276576620:BASE:276576707:1571955401309:74990b4e

ivy presubmit = https://test.corp.google.com/ui#id=OCL:276576620:BASE:276577312:1571955603290:4d17c890

matsko · 2019-10-24T22:54:41Z

Presubmit = https://tap.corp.google.com/ui#id=OCL:276582512:BASE:276582536:1571957195235:cece96f5

Ivy Presubmit = https://test.corp.google.com/ui#id=OCL:276582512:BASE:276583791:1571957633346:28ed44c0

This patch ensures that the `[style]` and `[class]` based bindings are directly applied to an element's style and className attributes. This patch optimizes the algorithm so that it... - Doesn't construct an update an instance of `StylingMapArray` for `[style]` and `[class]` bindings - Doesn't apply `[style]` and `[class]` based entries using `classList` and `style` (direct attributes are used instead) - Doesn't split or iterate over all string-based tokens in a string value obtained from a `[class]` binding. This patch speeds up the following cases: - `<div [class]>` and `<div class="..." [class]>` - `<div [style]>` and `<div style="..." [style]>` The overall speec increase is by over 5x.

…assName

ngbot · 2019-10-25T00:39:48Z

I see that you just added the PR action: merge label, but the following checks are still failing:
status "google3" is failing
1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

matsko · 2019-10-25T00:40:33Z

merge-assistance: approved by misko and approved over slack by Pawel

ngbot · 2019-10-25T00:40:45Z

I see that you just added the PR action: merge label, but the following checks are still failing:
status "google3" is failing
1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

AndrewKushnir · 2019-10-25T00:41:49Z

Most recent presubmits (for history):

Presubmits are successful.

…angular#33336) This patch ensures that the `[style]` and `[class]` based bindings are directly applied to an element's style and className attributes. This patch optimizes the algorithm so that it... - Doesn't construct an update an instance of `StylingMapArray` for `[style]` and `[class]` bindings - Doesn't apply `[style]` and `[class]` based entries using `classList` and `style` (direct attributes are used instead) - Doesn't split or iterate over all string-based tokens in a string value obtained from a `[class]` binding. This patch speeds up the following cases: - `<div [class]>` and `<div class="..." [class]>` - `<div [style]>` and `<div style="..." [style]>` The overall speec increase is by over 5x. PR Close angular#33336

trotyl · 2019-10-25T11:40:49Z

packages/core/src/render3/styling/styling_debug.ts

+    // that are no longer active on the element.
+    return createProxy({
+      get(target: {}, prop: string): DebugNodeStylingEntry{
+        let value: DebugNodeStylingEntry = entries[prop]; if (!value) {


The position of if looks weird.

…angular#33336) This patch ensures that the `[style]` and `[class]` based bindings are directly applied to an element's style and className attributes. This patch optimizes the algorithm so that it... - Doesn't construct an update an instance of `StylingMapArray` for `[style]` and `[class]` bindings - Doesn't apply `[style]` and `[class]` based entries using `classList` and `style` (direct attributes are used instead) - Doesn't split or iterate over all string-based tokens in a string value obtained from a `[class]` binding. This patch speeds up the following cases: - `<div [class]>` and `<div class="..." [class]>` - `<div [style]>` and `<div style="..." [style]>` The overall speec increase is by over 5x. PR Close angular#33336

angular-automatic-lock-bot · 2019-11-25T16:11:50Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

matsko added target: major This PR is targeted for the next major release comp: ivy labels Oct 22, 2019

ngbot bot modified the milestone: needsTriage Oct 22, 2019

googlebot added the cla: yes label Oct 22, 2019

matsko changed the title ~~perf(ivy): apply [class] bindings directly to className~~ perf(ivy): apply [style]/[class] bindings directly to style/className Oct 22, 2019

matsko force-pushed the class_optimizations branch from 5cc4e12 to de4b21d Compare October 22, 2019 23:35

mhevery reviewed Oct 22, 2019

View reviewed changes

matsko force-pushed the class_optimizations branch from de4b21d to 0911796 Compare October 22, 2019 23:39

mhevery reviewed Oct 23, 2019

View reviewed changes

matsko marked this pull request as ready for review October 23, 2019 07:24

matsko requested a review from a team as a code owner October 23, 2019 07:24

matsko force-pushed the class_optimizations branch 3 times, most recently from 71a29db to e40ea58 Compare October 23, 2019 07:42

mhevery approved these changes Oct 23, 2019

View reviewed changes

matsko force-pushed the class_optimizations branch 2 times, most recently from 0a44db0 to 416aa5e Compare October 24, 2019 04:39

matsko requested a review from a team as a code owner October 24, 2019 04:39

matsko force-pushed the class_optimizations branch from 416aa5e to b3f437f Compare October 24, 2019 04:52

matsko force-pushed the class_optimizations branch 4 times, most recently from 3801c3b to 8371baf Compare October 24, 2019 05:34

matsko force-pushed the class_optimizations branch from 8371baf to 4d5c75c Compare October 24, 2019 05:59

matsko force-pushed the class_optimizations branch from 23d0a66 to 8eeb33b Compare October 24, 2019 19:01

mhevery approved these changes Oct 24, 2019

View reviewed changes

pkozlowski-opensource reviewed Oct 24, 2019

View reviewed changes

matsko force-pushed the class_optimizations branch 4 times, most recently from 72aab4d to 6df1a2f Compare October 24, 2019 22:15

matsko force-pushed the class_optimizations branch from 6df1a2f to 1ee7a1a Compare October 24, 2019 22:45

matsko added 2 commits October 24, 2019 16:30

fixup! perf(ivy): apply [style]/[class] bindings directly to style/cl…

82149a1

…assName

matsko force-pushed the class_optimizations branch from 1ee7a1a to 82149a1 Compare October 24, 2019 23:30

matsko added the action: merge The PR is ready for merge by the caretaker label Oct 25, 2019

matsko added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: merge The PR is ready for merge by the caretaker labels Oct 25, 2019

ngbot bot added the action: merge The PR is ready for merge by the caretaker label Oct 25, 2019

AndrewKushnir closed this in dcdb433 Oct 25, 2019

trotyl reviewed Oct 25, 2019

View reviewed changes

matsko deleted the class_optimizations branch October 25, 2019 17:08

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(ivy): apply [style]/[class] bindings directly to style/className #33336

perf(ivy): apply [style]/[class] bindings directly to style/className #33336

matsko commented Oct 22, 2019 •

edited by mhevery

matsko commented Oct 23, 2019

mhevery Oct 23, 2019

matsko Oct 23, 2019

IgorMinar commented Oct 23, 2019

IgorMinar commented Oct 23, 2019

mhevery left a comment

matsko commented Oct 24, 2019

matsko commented Oct 24, 2019

matsko commented Oct 24, 2019

mhevery Oct 24, 2019

mhevery Oct 24, 2019

pkozlowski-opensource Oct 24, 2019

pkozlowski-opensource Oct 24, 2019

matsko commented Oct 24, 2019

matsko commented Oct 24, 2019

ngbot bot commented Oct 25, 2019

matsko commented Oct 25, 2019

ngbot bot commented Oct 25, 2019

AndrewKushnir commented Oct 25, 2019

trotyl Oct 25, 2019

angular-automatic-lock-bot bot commented Nov 25, 2019

perf(ivy): apply [style]/[class] bindings directly to style/className #33336

perf(ivy): apply [style]/[class] bindings directly to style/className #33336

Conversation

matsko commented Oct 22, 2019 • edited by mhevery

matsko commented Oct 23, 2019

mhevery Oct 23, 2019

Choose a reason for hiding this comment

matsko Oct 23, 2019

Choose a reason for hiding this comment

IgorMinar commented Oct 23, 2019

IgorMinar commented Oct 23, 2019

mhevery left a comment

Choose a reason for hiding this comment

matsko commented Oct 24, 2019

matsko commented Oct 24, 2019

matsko commented Oct 24, 2019

mhevery Oct 24, 2019

Choose a reason for hiding this comment

mhevery Oct 24, 2019

Choose a reason for hiding this comment

pkozlowski-opensource Oct 24, 2019

Choose a reason for hiding this comment

pkozlowski-opensource Oct 24, 2019

Choose a reason for hiding this comment

matsko commented Oct 24, 2019

matsko commented Oct 24, 2019

ngbot bot commented Oct 25, 2019

matsko commented Oct 25, 2019

ngbot bot commented Oct 25, 2019

AndrewKushnir commented Oct 25, 2019

trotyl Oct 25, 2019

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Nov 25, 2019

matsko commented Oct 22, 2019 •

edited by mhevery