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 enter view refactor #32263

Conversation

@pkozlowski-opensource
Copy link
Member

commented Aug 22, 2019

2 refactors that:

  • reduce time spent in the noop CD benchmark by ~12% (this is due to avoiding reads of tView.bindingStartIndex / writes of lView[BINDING_INDEX];
  • reduce concepts / code size

@googlebot googlebot added the cla: yes label Aug 22, 2019

@ngbot ngbot bot added this to the needsTriage milestone Aug 22, 2019

@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review Aug 22, 2019

@pkozlowski-opensource pkozlowski-opensource requested a review from angular/fw-core as a code owner Aug 22, 2019

@mhevery
Copy link
Member

left a comment

Consider making arguments required.

*/
export function enterView(newView: LView, hostTNode: TElementNode | TViewNode | null): LView {
export function selectView(
newView: LView, hostTNode: TElementNode | TViewNode | null = null): LView {

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 22, 2019

Member

There is a small penalty to calling a function with wrong number of arguments. In your case having last arg optional falls into this category. For something perf sensitive such as selectView I would consider making all arguments required.

perf test shows about 5% penalty for simple case.

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Aug 22, 2019

Author Member

Oh, I see. Will make it mandatory, thnx!

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Aug 26, 2019

Author Member

Done, the second arg is required now.

@kara
kara approved these changes Aug 23, 2019
Copy link
Contributor

left a comment

LGTM

perf(ivy): minimise writes to the lView[BINDING_INDEX] / binding root
This commit removes all the (duplicated) logic of setting lView[BINDING_INDEX]
from `enterView`. `enterView` is on the critcal path perf-wise so we should
avoid having any logic in there and minimise memory read / write.

This simple refactoring in this PR reduces time spent in noop change detection
by ~12% (from ~800ms down to ~700ms on a local machine where measurements were
taken).
refactor(ivy): replace enter / leave view with selectView
After a series of recent refactorings `enterView` and `leaveView` became
identical. This PR merges both into one concept of view selectio (similar
to a node selection). This reduces number of concepts and code size.

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:ivy_perf_enter_view_refactor branch from 996a3d7 to db8a78f Aug 26, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@atscott atscott closed this in e3422e0 Aug 26, 2019

atscott added a commit that referenced this pull request Aug 26, 2019
refactor(ivy): replace enter / leave view with selectView (#32263)
After a series of recent refactorings `enterView` and `leaveView` became
identical. This PR merges both into one concept of view selectio (similar
to a node selection). This reduces number of concepts and code size.

PR Close #32263
ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
perf(ivy): minimise writes to the lView[BINDING_INDEX] / binding root (
…angular#32263)

This commit removes all the (duplicated) logic of setting lView[BINDING_INDEX]
from `enterView`. `enterView` is on the critcal path perf-wise so we should
avoid having any logic in there and minimise memory read / write.

This simple refactoring in this PR reduces time spent in noop change detection
by ~12% (from ~800ms down to ~700ms on a local machine where measurements were
taken).

PR Close angular#32263
ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
refactor(ivy): replace enter / leave view with selectView (angular#32263
)

After a series of recent refactorings `enterView` and `leaveView` became
identical. This PR merges both into one concept of view selectio (similar
to a node selection). This reduces number of concepts and code size.

PR Close angular#32263
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
perf(ivy): minimise writes to the lView[BINDING_INDEX] / binding root (
…angular#32263)

This commit removes all the (duplicated) logic of setting lView[BINDING_INDEX]
from `enterView`. `enterView` is on the critcal path perf-wise so we should
avoid having any logic in there and minimise memory read / write.

This simple refactoring in this PR reduces time spent in noop change detection
by ~12% (from ~800ms down to ~700ms on a local machine where measurements were
taken).

PR Close angular#32263
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
refactor(ivy): replace enter / leave view with selectView (angular#32263
)

After a series of recent refactorings `enterView` and `leaveView` became
identical. This PR merges both into one concept of view selectio (similar
to a node selection). This reduces number of concepts and code size.

PR Close angular#32263
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.