Skip to content

Commit

Permalink
feat(StateRegistry): improve perf for: .register() and `StateMatche…
Browse files Browse the repository at this point in the history
…r.find()` misses

The `.find()` function is pretty hot/frequently used.
When possible, a state is found using a property look up `this._states[name]`
If a state isn't found using an exact match, another attempt is made to do a glob match.
The glob match allows a future state such as `future.state.**` to be matched for any child such as `future.state.child.nested`.

This causes unnecessary overhead especially during bootup and registration.
Before registering a state, we first check to see if it already exists.
This change does three things:

1) Only state names that *might* be a glob are tested
2) The Globs for states with glob-like names are cached
3) Do not use globs in `.register()` when checking for "State foo is already defined"

Related to #27
Related to angular-ui/ui-router#3274
  • Loading branch information
christopherthielen committed Jan 31, 2017
1 parent 4193244 commit fdb3ab9
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 25 deletions.
9 changes: 4 additions & 5 deletions src/common/glob.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,13 @@ export class Glob {
return this.regexp.test('.' + name);
}

/** @deprecated whats the point? */
/** Returns true if the string has glob-like characters in it */
static is(text: string) {
return text.indexOf('*') > -1;
return !!/[!,*]+/.exec(text);
}

/** @deprecated whats the point? */
/** Returns a glob from the string, or null if the string isn't Glob-like */
static fromString(text: string) {
if (!this.is(text)) return null;
return new Glob(text);
return Glob.is(text) ? new Glob(text) : null;
}
}
8 changes: 5 additions & 3 deletions src/state/stateMatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import {isString} from "../common/predicates";
import {StateOrName} from "./interface";
import {State} from "./stateObject";
import {Glob} from "../common/glob";
import {values} from "../common/common";

export class StateMatcher {
Expand All @@ -25,8 +24,11 @@ export class StateMatcher {
if (state && (isStr || (!isStr && (state === stateOrName || state.self === stateOrName)))) {
return state;
} else if (isStr) {
let matches = values(this._states)
.filter(state => new Glob(state.name).matches(name));
let _states = values(this._states);
let matches = _states.filter(state =>
state.__stateObjectCache.nameGlob &&
state.__stateObjectCache.nameGlob.matches(name)
);

if (matches.length > 1) {
console.log(`stateMatcher.find: Found multiple matches for ${name} using glob: `, matches.map(match => match.name));
Expand Down
16 changes: 13 additions & 3 deletions src/state/stateObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import { Resolvable } from "../resolve/resolvable";
import { TransitionStateHookFn } from "../transition/interface";
import { TargetState } from "./targetState";
import { Transition } from "../transition/transition";
import { Glob } from "../common/glob";
import { isObject } from "../common/predicates";

/**
* Internal representation of a UI-Router state.
Expand Down Expand Up @@ -95,6 +97,12 @@ export class State {
{ state: (string|StateDeclaration), params: { [key: string]: any }}
);

/** @hidden */
__stateObjectCache: {
/** Might be null */
nameGlob?: Glob
};


/** @deprecated use State.create() */
constructor(config?: StateDeclaration) {
Expand All @@ -112,14 +120,16 @@ export class State {
static create(stateDecl: StateDeclaration): State {
let state = inherit(inherit(stateDecl, State.prototype)) as State;
stateDecl.$$state = () => state;
state['__stateObject'] = true;
state.self = stateDecl;
state.__stateObjectCache = {
nameGlob: Glob.fromString(state.name) // might return null
};
return state;
}

/** Predicate which returns true if the object is an internal State object */
/** Predicate which returns true if the object is an internal [[State]] object */
static isState = (obj: any): obj is State =>
obj['__stateObject'] === true;
isObject(obj['__stateObjectCache']);

/**
* Returns true if the provided parameter is the same state.
Expand Down
30 changes: 18 additions & 12 deletions src/state/stateQueueManager.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
/** @module state */ /** for typedoc */
import { extend, inherit, pluck, inArray } from "../common/common";
import { isString, isDefined } from "../common/predicates";
import { inArray } from "../common/common";
import { isString } from "../common/predicates";
import { StateDeclaration } from "./interface";
import { State } from "./stateObject";
import { StateBuilder } from "./stateBuilder";
import { StateRegistryListener, StateRegistry } from "./stateRegistry";
import { Disposable } from "../interface";
import { UrlRouter } from "../url/urlRouter";
import { prop } from "../common/hof";
import { StateMatcher } from "./stateMatcher";

/** @internalapi */
export class StateQueueManager implements Disposable {
queue: State[];
matcher: StateMatcher;

constructor(
private $registry: StateRegistry,
Expand All @@ -20,6 +22,7 @@ export class StateQueueManager implements Disposable {
public builder: StateBuilder,
public listeners: StateRegistryListener[]) {
this.queue = [];
this.matcher = $registry.matcher;
}

/** @internalapi */
Expand Down Expand Up @@ -47,36 +50,39 @@ export class StateQueueManager implements Disposable {
let registered: State[] = [], // states that got registered
orphans: State[] = [], // states that don't yet have a parent registered
previousQueueLength = {}; // keep track of how long the queue when an orphan was first encountered
const getState = (name) =>
this.states.hasOwnProperty(name) && this.states[name];

while (queue.length > 0) {
let state: State = queue.shift();
let name = state.name;
let result: State = builder.build(state);
let orphanIdx: number = orphans.indexOf(state);

if (result) {
let existingState = this.$registry.get(state.name);

if (existingState && existingState.name === state.name) {
throw new Error(`State '${state.name}' is already defined`);
let existingState = getState(name);
if (existingState && existingState.name === name) {
throw new Error(`State '${name}' is already defined`);
}

if (existingState && existingState.name === state.name + ".**") {
let existingFutureState = getState(name + ".**");
if (existingFutureState) {
// Remove future state of the same name
this.$registry.deregister(existingState);
this.$registry.deregister(existingFutureState);
}

states[state.name] = state;
states[name] = state;
this.attachRoute(state);
if (orphanIdx >= 0) orphans.splice(orphanIdx, 1);
registered.push(state);
continue;
}

let prev = previousQueueLength[state.name];
previousQueueLength[state.name] = queue.length;
let prev = previousQueueLength[name];
previousQueueLength[name] = queue.length;
if (orphanIdx >= 0 && prev === queue.length) {
// Wait until two consecutive iterations where no additional states were dequeued successfully.
// throw new Error(`Cannot register orphaned state '${state.name}'`);
// throw new Error(`Cannot register orphaned state '${name}'`);
queue.push(state);
return states;
} else if (orphanIdx < 0) {
Expand Down
2 changes: 1 addition & 1 deletion src/transition/rejectFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export class Rejection {
return `TransitionRejection(type: ${type}, message: ${message}, detail: ${detail})`;
}

toPromise() {
toPromise(): Promise<any> {
return extend(silentRejection(this), { _transitionRejection: this });
}

Expand Down
2 changes: 1 addition & 1 deletion src/vanilla/$injector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export const $injector = {
invoke: (fn: IInjectable, context?, locals?) => {
let all = extend({}, globals, locals || {});
let params = $injector.annotate(fn);
let ensureExist = assertPredicate(key => all.hasOwnProperty(key), key => `DI can't find injectable: '${key}'`);
let ensureExist = assertPredicate((key: string) => all.hasOwnProperty(key), key => `DI can't find injectable: '${key}'`);
let args = params.filter(ensureExist).map(x => all[x]);
if (isFunction(fn)) return fn.apply(context, args);
else return (fn as any[]).slice(-1)[0].apply(context, args);
Expand Down

0 comments on commit fdb3ab9

Please sign in to comment.