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(ivy): chain styling instructions #33837

Closed
wants to merge 1 commit into from

Conversation

@crisbeto
Copy link
Member

crisbeto commented Nov 15, 2019

Adds support for chaining of styleProp, classProp and stylePropInterpolateX instructions whenever possible which should help generate less code. Note that one complication here is for stylePropInterpolateX instructions where we have to break into multiple chains if there are other styling instructions inbetween the interpolations which helps maintain the execution order.

@googlebot googlebot added the cla: yes label Nov 15, 2019
@crisbeto crisbeto force-pushed the crisbeto:styling-instruction-chain branch 3 times, most recently from b2de2d6 to 4de440d Nov 15, 2019
@crisbeto crisbeto marked this pull request as ready for review Nov 15, 2019
@crisbeto crisbeto requested review from angular/fw-compiler as code owners Nov 15, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 15, 2019
@@ -25,10 +24,14 @@ const IMPORTANT_FLAG = '!important';
* A styling expression summary that is to be processed by the compiler
*/
export interface StylingInstruction {

This comment has been minimized.

Copy link
@matsko

matsko Nov 15, 2019

Member

Since we're only using one property on this interface, why not just return a array of StylingInstructionCall instances?

This comment has been minimized.

Copy link
@crisbeto

crisbeto Nov 18, 2019

Author Member

It's set up like this so that it's easier to group calls to the same instruction together. If we wanted to change it, we'd have to do something like StylingInstructionCall[][] instead, but that would mean looping through all the calls when we're trying to figure out whether to chain them.

This comment has been minimized.

Copy link
@matsko

matsko Nov 18, 2019

Member

ah I see. I missed the reference member. This is fine.

@@ -133,7 +135,7 @@ export function stylePropInternal(
*
* @codeGenApi
*/
export function ɵɵclassProp(className: string, value: boolean | null): void {
export function ɵɵclassProp(className: string, value: boolean | null): TsickleIssue1009 {

This comment has been minimized.

Copy link
@matsko

matsko Nov 15, 2019

Member

Add a comment explaining the return value and why it's an instance of TsickleIssue1009

This comment has been minimized.

Copy link
@crisbeto

crisbeto Nov 18, 2019

Author Member

Isn't this the point of the TsickleIssue1009 type in the first place? Also Andrew mentioned that this isn't an issue anymore so as soon as these changes land I'll clean out all the usages of the TsickleIssue1009 type.

@crisbeto crisbeto force-pushed the crisbeto:styling-instruction-chain branch from 4de440d to 0ae1352 Nov 18, 2019
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Nov 18, 2019

The feedback has been addressed @matsko.

@kara
kara approved these changes Nov 18, 2019
Copy link
Contributor

kara left a comment

LGTM aside from typos. Note that we should be careful with the G3 presubmit for this because we are changing binding order in some cases, which could break some G3 targets.

@matsko
matsko approved these changes Nov 18, 2019
@@ -25,10 +24,14 @@ const IMPORTANT_FLAG = '!important';
* A styling expression summary that is to be processed by the compiler
*/
export interface StylingInstruction {

This comment has been minimized.

Copy link
@matsko

matsko Nov 18, 2019

Member

ah I see. I missed the reference member. This is fine.

Adds support for chaining of `styleProp`, `classProp` and `stylePropInterpolateX` instructions whenever possible which should help generate less code. Note that one complication here is for `stylePropInterpolateX` instructions where we have to break into multiple chains if there are other styling instructions inbetween the interpolations which helps maintain the execution order.
@crisbeto crisbeto force-pushed the crisbeto:styling-instruction-chain branch from 0ae1352 to caa2a7d Nov 18, 2019
@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Nov 19, 2019

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

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.

@alxhub alxhub closed this in 8a052dc Nov 19, 2019
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this pull request Dec 11, 2019
Adds support for chaining of `styleProp`, `classProp` and `stylePropInterpolateX` instructions whenever possible which should help generate less code. Note that one complication here is for `stylePropInterpolateX` instructions where we have to break into multiple chains if there are other styling instructions inbetween the interpolations which helps maintain the execution order.

PR Close angular#33837
AndrewKushnir added a commit that referenced this pull request Dec 11, 2019
Adds support for chaining of `styleProp`, `classProp` and `stylePropInterpolateX` instructions whenever possible which should help generate less code. Note that one complication here is for `stylePropInterpolateX` instructions where we have to break into multiple chains if there are other styling instructions inbetween the interpolations which helps maintain the execution order.

PR Close #33837

PR Close #34340
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Dec 20, 2019

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.