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

Set of performance improvements #31839

Conversation

pkozlowski-opensource
Copy link
Member

No description provided.

@pkozlowski-opensource pkozlowski-opensource added comp: ivy action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Jul 25, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jul 25, 2019
@pkozlowski-opensource pkozlowski-opensource changed the title Benchmark and improvements Set of performance improvements Jul 25, 2019
@@ -22,6 +22,18 @@ Great reads:

See benchmark [here](https://jsperf.com/mono-vs-megamorphic-property-access).

## Packed vs. holey Array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding this!

const blueprint = ngDevMode ? new LViewBlueprint !(initialViewLength) :
Array.apply(null, Array(initialViewLength));

blueprint.fill(null, 0, bindingStartIndex).fill(NO_CHANGE, bindingStartIndex) as LView;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is much better way to do this.

const blueprint = ngDevMode ? new LViewBlueprint !() : [];
blueprint.fill(null, 0, bindingStartIndex).fill(NO_CHANGE, bindingStartIndex, initialViewLength) as LView;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work as .fill doesn't write past the array length. I've pushed an alternative (if we don't want to use the "apply" trick):

 const blueprint = ngDevMode ? new LViewBlueprint !() : [];

  for (let i = 0; i < initialViewLength; i++) {
    blueprint.push(i < bindingStartIndex ? null : NO_CHANGE);
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? OK.

@pkozlowski-opensource pkozlowski-opensource force-pushed the benchmark_and_improvements branch 2 times, most recently from 9a738d8 to 98a4a57 Compare July 26, 2019 12:30
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review July 26, 2019 14:21
@pkozlowski-opensource pkozlowski-opensource requested a review from a team as a code owner July 26, 2019 14:21
@mhevery mhevery removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 26, 2019
@pkozlowski-opensource pkozlowski-opensource added the action: merge The PR is ready for merge by the caretaker label Jul 29, 2019
@ngbot
Copy link

ngbot bot commented Jul 29, 2019

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

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 AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 29, 2019
@AndrewKushnir
Copy link
Contributor

Adding "cleanup" label to indicate that the size limit needs an update (as per discussion due the team sync), so that test_aio_local_ivy CircleCI task passes.

@pkozlowski-opensource
Copy link
Member Author

pkozlowski-opensource commented Jul 30, 2019

@AndrewKushnir I've updated the size limit, as discussed. I did it in a separate commit and update to the same value as in the @crisbeto PR #31845 so we are not generating conflicts (or can drop the commit in question if needed).

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perf(aio): update main bundle size

This does not seem like the correct commit message.

  • the subject we use is now docs-infra (aio is deprecated as a subject)
  • should it be type perf? I would expect build or ci or even test

Also it would be really helpful for future readers if you explain in the commit message why this has been done (and mention the discussion).

@petebacondarwin petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 30, 2019
PR angular#31839 contains perf and code cleanup changes that add 48 bytes to the
main bundle size. Perf gains and code clarity justify this increase.

Unfortunately the size verification job is rejecting this PR as it reports
size gains from a fixed size and not relative increase of size from a particular PR.

It was decided during the internal team discussion to bump up size limits to
correctly reflect current state of the master and increase from this PR.
@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Jul 30, 2019
@AndrewKushnir
Copy link
Contributor

@pkozlowski-opensource pkozlowski-opensource removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 30, 2019
@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jul 30, 2019
AndrewKushnir pushed a commit that referenced this pull request Jul 30, 2019
AndrewKushnir pushed a commit that referenced this pull request Jul 30, 2019
…31839)

setPreviousOrParentTNode is set in enterView so no need to reset it
just before entering a view.

PR Close #31839
AndrewKushnir pushed a commit that referenced this pull request Jul 30, 2019
AndrewKushnir pushed a commit that referenced this pull request Jul 30, 2019
AndrewKushnir pushed a commit that referenced this pull request Jul 30, 2019
AndrewKushnir pushed a commit that referenced this pull request Jul 30, 2019
AndrewKushnir pushed a commit that referenced this pull request Jul 30, 2019
PR #31839 contains perf and code cleanup changes that add 48 bytes to the
main bundle size. Perf gains and code clarity justify this increase.

Unfortunately the size verification job is rejecting this PR as it reports
size gains from a fixed size and not relative increase of size from a particular PR.

It was decided during the internal team discussion to bump up size limits to
correctly reflect current state of the master and increase from this PR.

PR Close #31839
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
…ngular#31839)

setPreviousOrParentTNode is set in enterView so no need to reset it
just before entering a view.

PR Close angular#31839
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
PR angular#31839 contains perf and code cleanup changes that add 48 bytes to the
main bundle size. Perf gains and code clarity justify this increase.

Unfortunately the size verification job is rejecting this PR as it reports
size gains from a fixed size and not relative increase of size from a particular PR.

It was decided during the internal team discussion to bump up size limits to
correctly reflect current state of the master and increase from this PR.

PR Close angular#31839
@angular-automatic-lock-bot
Copy link

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 Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants