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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
652f090
perf(ivy): apply [style]/[class] bindings directly to style/className
matsko Oct 22, 2019
82149a1
fixup! perf(ivy): apply [style]/[class] bindings directly to style/cl…
matsko Oct 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/common/src/directives/ng_class.ts
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ export const ngClassDirectiveDef__POST_R3__ = ɵɵdefineDirective({
selectors: null as any,
hostBindings: function(rf: ɵRenderFlags, ctx: any, elIndex: number) {
if (rf & ɵRenderFlags.Create) {
ɵɵallocHostVars(1);
ɵɵallocHostVars(2);
}
if (rf & ɵRenderFlags.Update) {
ɵɵclassMap(ctx.getValue());
Expand Down
2 changes: 1 addition & 1 deletion packages/common/src/directives/ng_style.ts
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@ export const ngStyleDirectiveDef__POST_R3__ = ɵɵdefineDirective({
selectors: null as any,
hostBindings: function(rf: ɵRenderFlags, ctx: any, elIndex: number) {
if (rf & ɵRenderFlags.Create) {
ɵɵallocHostVars(1);
ɵɵallocHostVars(2);
}
if (rf & ɵRenderFlags.Update) {
ɵɵstyleMap(ctx.getValue());
Expand Down
32 changes: 16 additions & 16 deletions packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts
View file
Open in desktop
Expand Up @@ -436,7 +436,7 @@ describe('compiler compliance: styling', () => {
const template = `
…
decls: 1,
vars: 2,
vars: 3,
template: function MyComponentWithInterpolation_Template(rf, $ctx$) {
if (rf & 1) {
$r3$.ɵɵelement(0, "div");
Expand All @@ -447,7 +447,7 @@ describe('compiler compliance: styling', () => {
}
…
decls: 1,
vars: 3,
vars: 4,
template: function MyComponentWithMuchosInterpolation_Template(rf, $ctx$) {
if (rf & 1) {
$r3$.ɵɵelement(0, "div");
Expand All @@ -458,7 +458,7 @@ describe('compiler compliance: styling', () => {
}
…
decls: 1,
vars: 1,
vars: 2,
template: function MyComponentWithoutInterpolation_Template(rf, $ctx$) {
if (rf & 1) {
$r3$.ɵɵelement(0, "div");
Expand Down Expand Up @@ -506,7 +506,7 @@ describe('compiler compliance: styling', () => {
type: MyComponent,
selectors:[["my-component"]],
decls: 1,
vars: 4,
vars: 5,
consts: [[${AttributeMarker.Styles}, "opacity", "1"]],
template: function MyComponent_Template(rf, $ctx$) {
if (rf & 1) {
Expand Down Expand Up @@ -700,7 +700,7 @@ describe('compiler compliance: styling', () => {
type: MyComponent,
selectors:[["my-component"]],
decls: 1,
vars: 4,
vars: 5,
consts: [[${AttributeMarker.Classes}, "grape"]],
template: function MyComponent_Template(rf, $ctx$) {
if (rf & 1) {
Expand Down Expand Up @@ -863,8 +863,8 @@ describe('compiler compliance: styling', () => {
}
if (rf & 2) {
$r3$.ɵɵstyleSanitizer($r3$.ɵɵdefaultStyleSanitizer);
$r3$.ɵɵstyleMap($r3$.ɵɵpipeBind1(1, 2, $ctx$.myStyleExp));
$r3$.ɵɵclassMap($r3$.ɵɵpipeBind1(2, 4, $ctx$.myClassExp));
$r3$.ɵɵstyleMap($r3$.ɵɵpipeBind1(1, 4, $ctx$.myStyleExp));
$r3$.ɵɵclassMap($r3$.ɵɵpipeBind1(2, 6, $ctx$.myClassExp));
}
}
`;
Expand Down Expand Up @@ -916,11 +916,11 @@ describe('compiler compliance: styling', () => {
}
if (rf & 2) {
$r3$.ɵɵstyleSanitizer($r3$.ɵɵdefaultStyleSanitizer);
$r3$.ɵɵstyleMap($r3$.ɵɵpipeBind2(1, 6, $ctx$.myStyleExp, 1000));
$r3$.ɵɵstyleMap($r3$.ɵɵpipeBind2(1, 8, $ctx$.myStyleExp, 1000));
$r3$.ɵɵclassMap($e2_styling$);
$r3$.ɵɵstyleProp("bar", $r3$.ɵɵpipeBind2(2, 9, $ctx$.barExp, 3000));
$r3$.ɵɵstyleProp("baz", $r3$.ɵɵpipeBind2(3, 12, $ctx$.bazExp, 4000));
$r3$.ɵɵclassProp("foo", $r3$.ɵɵpipeBind2(4, 15, $ctx$.fooExp, 2000));
$r3$.ɵɵstyleProp("bar", $r3$.ɵɵpipeBind2(2, 11, $ctx$.barExp, 3000));
$r3$.ɵɵstyleProp("baz", $r3$.ɵɵpipeBind2(3, 14, $ctx$.bazExp, 4000));
$r3$.ɵɵclassProp("foo", $r3$.ɵɵpipeBind2(4, 17, $ctx$.fooExp, 2000));
$r3$.ɵɵadvance(5);
$r3$.ɵɵtextInterpolate1(" ", $ctx$.item, "");
}
Expand Down Expand Up @@ -1018,7 +1018,7 @@ describe('compiler compliance: styling', () => {
const template = `
hostBindings: function MyComponent_HostBindings(rf, ctx, elIndex) {
if (rf & 1) {
$r3$.ɵɵallocHostVars(4);
$r3$.ɵɵallocHostVars(6);
$r3$.ɵɵelementHostAttrs($e0_attrs$);
}
if (rf & 2) {
Expand Down Expand Up @@ -1077,7 +1077,7 @@ describe('compiler compliance: styling', () => {
const template = `
hostBindings: function MyComponent_HostBindings(rf, ctx, elIndex) {
if (rf & 1) {
$r3$.ɵɵallocHostVars(6);
$r3$.ɵɵallocHostVars(8);
}
if (rf & 2) {
$r3$.ɵɵstyleSanitizer($r3$.ɵɵdefaultStyleSanitizer);
Expand Down Expand Up @@ -1152,7 +1152,7 @@ describe('compiler compliance: styling', () => {
const hostBindings = `
hostBindings: function MyComponent_HostBindings(rf, ctx, elIndex) {
if (rf & 1) {
$r3$.ɵɵallocHostVars(4);
$r3$.ɵɵallocHostVars(6);
}
if (rf & 2) {
$r3$.ɵɵstyleSanitizer($r3$.ɵɵdefaultStyleSanitizer);
Expand Down Expand Up @@ -1218,7 +1218,7 @@ describe('compiler compliance: styling', () => {
const template = `
function ClassDirective_HostBindings(rf, ctx, elIndex) {
if (rf & 1) {
$r3$.ɵɵallocHostVars(1);
$r3$.ɵɵallocHostVars(2);
}
if (rf & 2) {
$r3$.ɵɵclassMap(ctx.myClassMap);
Expand Down Expand Up @@ -1506,7 +1506,7 @@ describe('compiler compliance: styling', () => {
…
hostBindings: function MyComponent_HostBindings(rf, ctx, elIndex) {
if (rf & 1) {
$r3$.ɵɵallocHostVars(4);
$r3$.ɵɵallocHostVars(6);
$r3$.ɵɵelementHostAttrs($_c0$);
}
if (rf & 2) {
Expand Down
5 changes: 4 additions & 1 deletion packages/compiler/src/render3/view/styling_builder.ts
View file
Open in desktop
Expand Up @@ -326,7 +326,10 @@ export class StylingBuilder {
valueConverter: ValueConverter, isClassBased: boolean,
stylingInput: BoundStylingEntry): StylingInstruction {
// each styling binding value is stored in the LView
let totalBindingSlotsRequired = 1;
// map-based bindings allocate two slots: one for the
// previous binding value and another for the previous
// className or style attribute value.
let totalBindingSlotsRequired = 2;

// these values must be outside of the update block so that they can
// be evaluated (the AST visit call) during creation time so that any
Expand Down
74 changes: 36 additions & 38 deletions packages/core/src/render3/instructions/styling.ts
View file
Open in desktop
Expand Up @@ -96,7 +96,7 @@ export function stylePropInternal(
// in this case we do not need to do anything, but the binding index
// still needs to be incremented because all styling binding values
// are stored inside of the lView.
const bindingIndex = lView[BINDING_INDEX]++;
const bindingIndex = getAndIncrementBindingIndex(lView, false);

const updated =
stylingProp(elementIndex, bindingIndex, prop, resolveStylePropValue(value, suffix), false);
Expand Down Expand Up @@ -130,7 +130,7 @@ export function ɵɵclassProp(className: string, value: boolean | null): void {
// in this case we do not need to do anything, but the binding index
// still needs to be incremented because all styling binding values
// are stored inside of the lView.
const bindingIndex = lView[BINDING_INDEX]++;
const bindingIndex = getAndIncrementBindingIndex(lView, false);

const updated = stylingProp(getSelectedIndex(), bindingIndex, className, value, true);
if (ngDevMode) {
Expand Down Expand Up @@ -189,8 +189,7 @@ function stylingProp(
const sanitizerToUse = isClassBased ? null : sanitizer;
const renderer = getRenderer(tNode, lView);
updated = applyStylingValueDirectly(
renderer, context, native, lView, bindingIndex, prop, value, isClassBased,
isClassBased ? setClass : setStyle, sanitizerToUse);
renderer, context, native, lView, bindingIndex, prop, value, isClassBased, sanitizerToUse);

if (sanitizerToUse) {
// it's important we remove the current style sanitizer once the
Expand Down Expand Up @@ -243,28 +242,23 @@ export function ɵɵstyleMap(styles: {[styleName: string]: any} | NO_CHANGE | nu
const lView = getLView();
const tNode = getTNode(index, lView);
const context = getStylesContext(tNode);
const hasDirectiveInput = hasStyleInput(tNode);

// if a value is interpolated then it may render a `NO_CHANGE` value.
// in this case we do not need to do anything, but the binding index
// still needs to be incremented because all styling binding values
// are stored inside of the lView.
const bindingIndex = lView[BINDING_INDEX]++;
const bindingIndex = getAndIncrementBindingIndex(lView, true);

// inputs are only evaluated from a template binding into a directive, therefore,
// there should not be a situation where a directive host bindings function
// evaluates the inputs (this should only happen in the template function)
if (!isHostStyling() && hasStyleInput(tNode) && styles !== NO_CHANGE) {
if (!isHostStyling() && hasDirectiveInput && styles !== NO_CHANGE) {
updateDirectiveInputValue(context, lView, tNode, bindingIndex, styles, false);
styles = NO_CHANGE;
}

const updated = stylingMap(index, context, bindingIndex, styles, false);
if (ngDevMode) {
ngDevMode.styleMap++;
if (updated) {
ngDevMode.styleMapCacheMiss++;
}
}
stylingMap(context, tNode, lView, bindingIndex, styles, false, hasDirectiveInput);
}

/**
Expand Down Expand Up @@ -300,28 +294,23 @@ export function classMapInternal(
const lView = getLView();
const tNode = getTNode(elementIndex, lView);
const context = getClassesContext(tNode);
const hasDirectiveInput = hasClassInput(tNode);

// if a value is interpolated then it may render a `NO_CHANGE` value.
// in this case we do not need to do anything, but the binding index
// still needs to be incremented because all styling binding values
// are stored inside of the lView.
const bindingIndex = lView[BINDING_INDEX]++;
const bindingIndex = getAndIncrementBindingIndex(lView, true);

// inputs are only evaluated from a template binding into a directive, therefore,
// there should not be a situation where a directive host bindings function
// evaluates the inputs (this should only happen in the template function)
if (!isHostStyling() && hasClassInput(tNode) && classes !== NO_CHANGE) {
if (!isHostStyling() && hasDirectiveInput && classes !== NO_CHANGE) {
updateDirectiveInputValue(context, lView, tNode, bindingIndex, classes, true);
classes = NO_CHANGE;
}

const updated = stylingMap(elementIndex, context, bindingIndex, classes, true);
if (ngDevMode) {
ngDevMode.classMap++;
if (updated) {
ngDevMode.classMapCacheMiss++;
}
}
stylingMap(context, tNode, lView, bindingIndex, classes, true, hasDirectiveInput);
}

/**
Expand All @@ -331,13 +320,10 @@ export function classMapInternal(
* `[class]` bindings in Angular.
*/
function stylingMap(
elementIndex: number, context: TStylingContext, bindingIndex: number,
value: {[key: string]: any} | string | null, isClassBased: boolean): boolean {
let updated = false;

const lView = getLView();
context: TStylingContext, tNode: TNode, lView: LView, bindingIndex: number,
value: {[key: string]: any} | string | null, isClassBased: boolean,
hasDirectiveInput: boolean): void {
const directiveIndex = getActiveDirectiveId();
const tNode = getTNode(elementIndex, lView);
const native = getNativeByTNode(tNode, lView) as RElement;
const oldValue = getValue(lView, bindingIndex);
const hostBindingsMode = isHostStyling();
Expand All @@ -359,25 +345,24 @@ function stylingMap(
patchConfig(context, TStylingConfig.HasMapBindings);
}

const stylingMapArr =
value === NO_CHANGE ? NO_CHANGE : normalizeIntoStylingMap(oldValue, value, !isClassBased);

// Direct Apply Case: bypass context resolution and apply the
// style/class map values directly to the element
if (allowDirectStyling(context, hostBindingsMode)) {
const sanitizerToUse = isClassBased ? null : sanitizer;
const renderer = getRenderer(tNode, lView);
updated = applyStylingMapDirectly(
renderer, context, native, lView, bindingIndex, stylingMapArr as StylingMapArray,
isClassBased, isClassBased ? setClass : setStyle, sanitizerToUse, valueHasChanged);
applyStylingMapDirectly(
renderer, context, native, lView, bindingIndex, value, isClassBased, sanitizerToUse,
valueHasChanged, hasDirectiveInput);
if (sanitizerToUse) {
// it's important we remove the current style sanitizer once the
// element exits, otherwise it will be used by the next styling
// instructions for the next element.
setElementExitFn(stylingApply);
}
} else {
updated = valueHasChanged;
const stylingMapArr =
value === NO_CHANGE ? NO_CHANGE : normalizeIntoStylingMap(oldValue, value, !isClassBased);

activateStylingMapFeature();

// Context Resolution (or first update) Case: save the map value
Expand All @@ -396,7 +381,12 @@ function stylingMap(
setElementExitFn(stylingApply);
}

return updated;
if (ngDevMode) {
isClassBased ? ngDevMode.classMap : ngDevMode.styleMap++;
if (valueHasChanged) {
isClassBased ? ngDevMode.classMapCacheMiss : ngDevMode.styleMapCacheMiss++;
}
}
}

/**
Expand Down Expand Up @@ -452,8 +442,8 @@ function normalizeStylingDirectiveInputValue(
value = concatString(initialValue, forceClassesAsString(bindingValue));
} else {
value = concatString(
initialValue, forceStylesAsString(bindingValue as{[key: string]: any} | null | undefined),
';');
initialValue,
forceStylesAsString(bindingValue as{[key: string]: any} | null | undefined, true), ';');
}
}
return value;
Expand Down Expand Up @@ -594,3 +584,11 @@ function resolveStylePropValue(
function isHostStyling(): boolean {
return isHostStylingActive(getActiveDirectiveId());
}

function getAndIncrementBindingIndex(lView: LView, isMapBased: boolean): number {
// map-based bindings use two slots because the previously constructed
// className / style value must be compared against.
const index = lView[BINDING_INDEX];
lView[BINDING_INDEX] += isMapBased ? 2 : 1;
return index;
}