Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

[WIP] perf(View): Improve View instantiation speed and memory consumption. #1178

Merged
merged 1 commit into from
Jul 22, 2014

Conversation

mhevery
Copy link
Contributor

@mhevery mhevery commented Jun 25, 2014

View contains many injectors which are expensive in both speed and memory. The
New DirectiveInjector assumes that there are no more than 10 directives
per element and can be a lot more efficient than a array/hash lookup and
those it can be faster as well as smaller. This change makes View
instantiation speed 4x faster in Dartium VM.

BREAKING CHANGE:

  • Injector no longer supports visibility
  • The Directive:module instead of returning Module now takes
    DirectiveModule (which supports visibility)
  • Application Injector and DirectiveInjector now have separate trees.
    (The root of DirectiveInjector is ApplicationInjector)

@mhevery mhevery added cla: yes and removed cla: no labels Jun 25, 2014
@@ -36,8 +36,10 @@ packages:
source: hosted
version: "0.9.2"
di:
description: di
source: hosted
description:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to merge this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we don't not final, work in progress. :-)

@vicb vicb changed the title perf(View): Improve View instantiation speed and memory consumption. [WIP] perf(View): Improve View instantiation speed and memory consumption. Jun 26, 2014
=> injector.name == SHADOW_DOM_INJECTOR_NAME ? injector.parent : injector;
abstract class DirectiveBinder {
bind(key, {Function toFactory, Factory toFactoryPos, inject,
Visibility visibility: Visibility.DIRECT_CHILD});
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this Visibility.LOCAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

On Thu, Jun 26, 2014 at 3:07 PM, Yegor notifications@github.com wrote:

In lib/core/annotation_src.dart:

@@ -2,35 +2,28 @@ library angular.core.annotation_src;

import "package:di/di.dart" show Injector, Visibility;

-RegExp _ATTR_NAME = new RegExp(r'[([^]]+)]$');

-const String SHADOW_DOM_INJECTOR_NAME = 'SHADOW_INJECTOR';

-skipShadow(Injector injector)

  • => injector.name == SHADOW_DOM_INJECTOR_NAME ? injector.parent : injector;
    +abstract class DirectiveBinder {
  • bind(key, {Function toFactory, Factory toFactoryPos, inject,
  •         Visibility visibility: Visibility.DIRECT_CHILD});
    

Make this Visibility.LOCAL


Reply to this email directly or view it on GitHub
https://github.com/angular/angular.dart/pull/1178/files#r14269780.

@jrote1
Copy link
Contributor

jrote1 commented Jul 14, 2014

When will this be merged?

jbdeboer added a commit that referenced this pull request Jul 14, 2014
jbdeboer added a commit that referenced this pull request Jul 14, 2014
@rkirov rkirov mentioned this pull request Jul 14, 2014
dsalsbury pushed a commit to dsalsbury/angular.dart that referenced this pull request Jul 16, 2014
…o allow

gradual migration.

Originally authored by @yjbanov
View contains many injectors which are expensive in both speed and memory. The
New DirectiveInjector assumes that there are no more than 10 directives
per element and can be a lot more efficient than a array/hash lookup and
those it can be faster as well as smaller. This change makes View
instantiation speed 4x faster in Dartium VM.

BREAKING CHANGE:

- Injector no longer supports visibility
- The Directive:module instead of returning Module now takes
  DirectiveModule (which supports visibility)
- Application Injector and DirectiveInjector now have separate trees.
  (The root if DirectiveInjector is ApplicationInjector)
@vicb
Copy link
Contributor

vicb commented Jul 21, 2014

@mhevery I have submitted a PR to your repo to update the branch, see Remove usage of (deprecated) probe

@chirayuk chirayuk merged commit 494deda into dart-archive:master Jul 22, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

7 participants