Skip to content

Commit

Permalink
perf(ivy): avoid unnecessary DOM reads in styling instructions (#32716)
Browse files Browse the repository at this point in the history
Before this refactoring native node `classList` / `style` properties were
read even if not used. The reason for this was desire to avoid code duplication
between procedural and non-procedural renderers. Unfortunatelly for the case
which will be used by most users (a procedura renderer) the `classList` / `style`
properties were read twice, making the `setStyle` \ `setClass` functions the
most expensive ones (self time) in several benchmarks (large table, expanding
rows).

This refactoring adds a bit of code duplication in order to get better
runtime performance. The code duplication will be removed when we drop
checks for a procedural renderer.

PR Close #32716
  • Loading branch information
pkozlowski-opensource authored and AndrewKushnir committed Sep 17, 2019
1 parent 3ace25f commit 05e1b3b
Showing 1 changed file with 52 additions and 29 deletions.
81 changes: 52 additions & 29 deletions packages/core/src/render3/styling_next/bindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -712,25 +712,37 @@ export function setStylingMapsSyncFn(fn: SyncStylingMapsFn) {
*/
export const setStyle: ApplyStylingFn =
(renderer: Renderer3 | null, native: RElement, prop: string, value: string | null) => {
// the reason why this may be `null` is either because
// it's a container element or it's a part of a test
// environment that doesn't have styling. In either
// case it's safe not to apply styling to the element.
const nativeStyle = native.style;
if (value) {
// opacity, z-index and flexbox all have number values
// and these need to be converted into strings so that
// they can be assigned properly.
value = value.toString();
ngDevMode && ngDevMode.rendererSetStyle++;
renderer && isProceduralRenderer(renderer) ?
renderer.setStyle(native, prop, value, RendererStyleFlags3.DashCase) :
(nativeStyle && nativeStyle.setProperty(prop, value));
} else {
ngDevMode && ngDevMode.rendererRemoveStyle++;
renderer && isProceduralRenderer(renderer) ?
renderer.removeStyle(native, prop, RendererStyleFlags3.DashCase) :
(nativeStyle && nativeStyle.removeProperty(prop));
if (renderer !== null) {
if (value) {
// opacity, z-index and flexbox all have number values
// and these need to be converted into strings so that
// they can be assigned properly.
value = value.toString();
ngDevMode && ngDevMode.rendererSetStyle++;
if (isProceduralRenderer(renderer)) {
renderer.setStyle(native, prop, value, RendererStyleFlags3.DashCase);
} else {
// The reason why native style may be `null` is either because
// it's a container element or it's a part of a test
// environment that doesn't have styling. In either
// case it's safe not to apply styling to the element.
const nativeStyle = native.style;
if (nativeStyle != null) {
nativeStyle.setProperty(prop, value);
}
}
} else {
ngDevMode && ngDevMode.rendererRemoveStyle++;

if (isProceduralRenderer(renderer)) {
renderer.removeStyle(native, prop, RendererStyleFlags3.DashCase);
} else {
const nativeStyle = native.style;
if (nativeStyle != null) {
nativeStyle.removeProperty(prop);
}
}
}
}
};

Expand All @@ -739,20 +751,31 @@ export const setStyle: ApplyStylingFn =
*/
export const setClass: ApplyStylingFn =
(renderer: Renderer3 | null, native: RElement, className: string, value: any) => {
if (className !== '') {
// the reason why this may be `null` is either because
// it's a container element or it's a part of a test
// environment that doesn't have styling. In either
// case it's safe not to apply styling to the element.
const classList = native.classList;
if (renderer !== null && className !== '') {
if (value) {
ngDevMode && ngDevMode.rendererAddClass++;
renderer && isProceduralRenderer(renderer) ? renderer.addClass(native, className) :
(classList && classList.add(className));
if (isProceduralRenderer(renderer)) {
renderer.addClass(native, className);
} else {
// the reason why classList may be `null` is either because
// it's a container element or it's a part of a test
// environment that doesn't have styling. In either
// case it's safe not to apply styling to the element.
const classList = native.classList;
if (classList != null) {
classList.add(className);
}
}
} else {
ngDevMode && ngDevMode.rendererRemoveClass++;
renderer && isProceduralRenderer(renderer) ? renderer.removeClass(native, className) :
(classList && classList.remove(className));
if (isProceduralRenderer(renderer)) {
renderer.removeClass(native, className);
} else {
const classList = native.classList;
if (classList != null) {
classList.remove(className);
}
}
}
}
};
Expand Down

0 comments on commit 05e1b3b

Please sign in to comment.