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

Ivy perf 2019 10 03 #32979

Conversation

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Oct 3, 2019

This PR has a couple of perf improvements that bring down time of the element_text_create benchmark from ~270ms to ~220ms (~18% time reduction). The speed-up is mostly achieved by guarding initial styling rendering by a TNode flag (render initial styling does quite a bit of memory reads on TNode and this is unnecessary when there is no initial styling).

@googlebot googlebot added the cla: yes label Oct 3, 2019
@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:ivy_perf_2019_10_03 branch from 40df99d to 563bc91 Oct 3, 2019
@atscott atscott added the comp: ivy label Oct 3, 2019
@ngbot ngbot bot added this to the needsTriage milestone Oct 3, 2019
@mhevery
mhevery approved these changes Oct 3, 2019
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review Oct 3, 2019
@pkozlowski-opensource pkozlowski-opensource requested a review from angular/fw-core as a code owner Oct 3, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Oct 3, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Oct 3, 2019

@pkozlowski-opensource Ivy presubmit indicated a regression in g3 (so I added the "blocked" label for now). I've put together a test case that illustrates the problem:

  it('should keep classes for all elements in *ngFor', () => {
    @Component({
      selector: '[comp]',
      template: '',
    })
    class Comp {}

    @Component({
      template: `
        <ng-container *ngFor="let item of items">
          <p comp class="a">A</p>
        </ng-container>
      `
    })
    class App {
      items = [1, 2, 3];
    }

    TestBed.configureTestingModule({
      declarations: [App, Comp],
    });
    const fixture = TestBed.createComponent(App);
    fixture.detectChanges();

    // This assertion fails in Ivy, since only the first element actually receives a class:
    // <p comp="" class="a"></p><!--ng-container-->
    // <p comp=""></p><!--ng-container-->
    // <p comp=""></p><!--ng-container-->
    expect(fixture.debugElement.queryAll(By.css('.a')).length).toBe(3);
  });

Could you please look into this problem?

Thank you.

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:ivy_perf_2019_10_03 branch from 844935e to 58b22b3 Oct 4, 2019
@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:ivy_perf_2019_10_03 branch from 58b22b3 to 5461e17 Oct 4, 2019
@pkozlowski-opensource

This comment has been minimized.

Copy link
Member Author

pkozlowski-opensource commented Oct 4, 2019

@AndrewKushnir thnx for digging into G3 failure! I've included your test in this PR and fixed the underlying issue.

Could you please re-run the G3 pre-submit?

@atscott

This comment has been minimized.

Copy link
Contributor

atscott commented Oct 4, 2019

@atscott

This comment has been minimized.

Copy link
Contributor

atscott commented Oct 4, 2019

Presubmits look good 👍

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Oct 4, 2019

@pkozlowski-opensource thanks for the fix! 👍

@alxhub alxhub closed this in 8593d0d Oct 4, 2019
alxhub added a commit that referenced this pull request Oct 4, 2019
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Nov 4, 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 Nov 4, 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.