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): avoid duplicate state lookup and default function parameters #34183

Closed

Conversation

@crisbeto
Copy link
Member

crisbeto commented Dec 2, 2019

Includes a few minor performance improvements:

  • In the nextContext instruction we assign a new LView to the LFrame.contextLView and then we immediately look it up to get its context. We don't need to do that since we know the view that was assigned already.
  • Removes the default value for the level parameter of nextContextImpl because it generates more code in es5 and is internal-only.
  • Removes the default parameter from setActiveHostElement since it generates extra code and it's an internal function.
  • Makes a check in setElementExitFn more strict since we're guaranteed for the value to match the type.
@googlebot googlebot added the cla: yes label Dec 2, 2019
Includes a few minor performance improvements:
* In the `nextContext` instruction we assign a new LView to the `LFrame.contextLView` and then we immediately look it up to get its context. We don't need to do that since we know the view that was assigned already.
* Removes the default value for the `level` parameter of `nextContextImpl` because it generates more code in es5 and is internal-only.
* Removes the default parameter from `setActiveHostElement` since it generates extra code and it's an internal function.
* Makes a check in `setElementExitFn` more strict since we're guaranteed for the value to match the type.
@crisbeto crisbeto force-pushed the crisbeto:next-context-duplicate-lookup branch from fe56625 to aceb42c Dec 2, 2019
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Dec 2, 2019

The integration_test failure will be fixed by #34179.

@crisbeto crisbeto marked this pull request as ready for review Dec 2, 2019
@crisbeto crisbeto requested a review from angular/fw-core as a code owner Dec 2, 2019
@ngbot ngbot bot added this to the needsTriage milestone Dec 2, 2019
@crisbeto crisbeto changed the title perf(ivy): avoid duplicate state lookup in nextContext instruction perf(ivy): avoid duplicate state lookup and default function parameters Dec 2, 2019
@mhevery
mhevery approved these changes Dec 2, 2019
@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Dec 2, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: integration_test" is failing
    pending status "google3" is pending
    pending missing required status "ci/circleci: publish_snapshot"

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.

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Dec 2, 2019

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Dec 3, 2019

@mhevery mhevery closed this in a295255 Dec 4, 2019
mhevery added a commit that referenced this pull request Dec 4, 2019
…rs (#34183)

Includes a few minor performance improvements:
* In the `nextContext` instruction we assign a new LView to the `LFrame.contextLView` and then we immediately look it up to get its context. We don't need to do that since we know the view that was assigned already.
* Removes the default value for the `level` parameter of `nextContextImpl` because it generates more code in es5 and is internal-only.
* Removes the default parameter from `setActiveHostElement` since it generates extra code and it's an internal function.
* Makes a check in `setElementExitFn` more strict since we're guaranteed for the value to match the type.

PR Close #34183
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.