From fdb3ab95d727e87a744362eff1ddf1cf98eea0a0 Mon Sep 17 00:00:00 2001 From: Chris Thielen Date: Tue, 31 Jan 2017 10:49:46 -0600 Subject: [PATCH] feat(StateRegistry): improve perf for: `.register()` and `StateMatcher.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 https://github.com/ui-router/core/issues/27 Related to https://github.com/angular-ui/ui-router/issues/3274 --- src/common/glob.ts | 9 ++++----- src/state/stateMatcher.ts | 8 +++++--- src/state/stateObject.ts | 16 +++++++++++++--- src/state/stateQueueManager.ts | 30 ++++++++++++++++++------------ src/transition/rejectFactory.ts | 2 +- src/vanilla/$injector.ts | 2 +- 6 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/common/glob.ts b/src/common/glob.ts index 4d3e5a96..3be9e559 100644 --- a/src/common/glob.ts +++ b/src/common/glob.ts @@ -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; } } diff --git a/src/state/stateMatcher.ts b/src/state/stateMatcher.ts index aa779851..143c95f2 100644 --- a/src/state/stateMatcher.ts +++ b/src/state/stateMatcher.ts @@ -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 { @@ -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)); diff --git a/src/state/stateObject.ts b/src/state/stateObject.ts index 7325f67c..d8cc0041 100644 --- a/src/state/stateObject.ts +++ b/src/state/stateObject.ts @@ -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. @@ -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) { @@ -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. diff --git a/src/state/stateQueueManager.ts b/src/state/stateQueueManager.ts index 2b1bf330..a868a276 100644 --- a/src/state/stateQueueManager.ts +++ b/src/state/stateQueueManager.ts @@ -1,6 +1,6 @@ /** @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"; @@ -8,10 +8,12 @@ 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, @@ -20,6 +22,7 @@ export class StateQueueManager implements Disposable { public builder: StateBuilder, public listeners: StateRegistryListener[]) { this.queue = []; + this.matcher = $registry.matcher; } /** @internalapi */ @@ -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) { diff --git a/src/transition/rejectFactory.ts b/src/transition/rejectFactory.ts index b2889e45..1ab47fbd 100644 --- a/src/transition/rejectFactory.ts +++ b/src/transition/rejectFactory.ts @@ -29,7 +29,7 @@ export class Rejection { return `TransitionRejection(type: ${type}, message: ${message}, detail: ${detail})`; } - toPromise() { + toPromise(): Promise { return extend(silentRejection(this), { _transitionRejection: this }); } diff --git a/src/vanilla/$injector.ts b/src/vanilla/$injector.ts index 9a213994..25637f81 100644 --- a/src/vanilla/$injector.ts +++ b/src/vanilla/$injector.ts @@ -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);