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

Slow Karma unit tests? #3274

Closed
jziggas opened this issue Jan 17, 2017 · 11 comments
Closed

Slow Karma unit tests? #3274

jziggas opened this issue Jan 17, 2017 · 11 comments
Milestone

Comments

@jziggas
Copy link

jziggas commented Jan 17, 2017

I'm wondering if anyone else has noticed something like this? We migrated our app recently from 0.2.x to ^1.0.0-rc.1 and have noticed that our unit tests are running super slow now. Our ~350 tests that took about 7 seconds to run before now takes 70 seconds and receives 'Slow' warnings from PhantomJS.

Going to see if upgrading PhantomJS from 2.1.1 helps.

PhantomJS 2.1.1
Karma 1.2
Jasmine 2.4.1

@justinbwarner
Copy link

Yes, I am seeing something similar. It appears to be related to how many states are in your app. Mine has ~50 states, the tests used to take about 3 seconds, now it takes more than 30. I noticed it because it exceeds the default timeouts for karma. If I comment-out a large number of the states then the tests run quickly.

@towu
Copy link

towu commented Jan 18, 2017

Maybe related to ui-router/core#27 (Performance of new Order URL Matching Rules by priority, not registration order)

@aj-dev
Copy link
Contributor

aj-dev commented Jan 18, 2017

Also noticed the same. Unit tests take 10x longer to run after migration from v0.3.2 to v1.0.0-rc.1.

Has something to do with state transitions. If I remove state definitions from the tests suite, it's back to normal.

@christopherthielen christopherthielen added this to the 1.0.0-final milestone Jan 19, 2017
@christopherthielen
Copy link
Contributor

I've noticed this as well. When I have time I will do some profiling to see where the most time is spent.

I would appreciate it somebody would profile it first and find the hot spots.

@justinbwarner
Copy link

Based on my limited profiling, it looks like one of the hot spots is in UrlRouter.prototype.sort (line 3736). This method seems to take 10 to 20 milliseconds for each state is created, so if you have a lot of states, loading them can take more than a second to process.

@christopherthielen
Copy link
Contributor

Great, thanks. That's low hanging fruit. I'll optimize that in next ui-router-core

@orneryd
Copy link

orneryd commented Jan 26, 2017

I think the easiest way to do this is to not invoke the sort fn until the $get is called for the $stateProvider
That way it gets called one time after all are registered up front. then on the Serivce instance side of the API call it when further states get registered (if needed)

@orneryd
Copy link

orneryd commented Jan 26, 2017

for now, what I did was what it says in the CHANGELOG.md

    //TODO: remove once the .sort is optimized in ui-router > 1.0.0-rc.1
    $urlServiceProvider.rules.sort(function(a, b) {
      return a.$id - b.$id;
    });

@christopherthielen christopherthielen modified the milestones: 1.0.0-rc.2, 1.0.0-final Jan 29, 2017
christopherthielen added a commit to ui-router/core that referenced this issue Jan 31, 2017
- extend: Use native ES6 `Object.assign` if available
- extend: Use nested for loop for non-native polyfill
- defaults: Simplify logic
- pick / omit: avoid v8 deopt. simplify. remove varargs support
- arrayTuples: unroll for arguments.length === 1, 2, 3, 4

Related to #27
Related to angular-ui/ui-router#3274
christopherthielen added a commit to ui-router/core that referenced this issue Jan 31, 2017
…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
@jziggas
Copy link
Author

jziggas commented Jan 31, 2017

😄

@christopherthielen
Copy link
Contributor

please report back whether or not these optimizations sped up your unit tests

@jziggas
Copy link
Author

jziggas commented Jul 18, 2017

Not 100% sure if related but I just upgraded to "@uirouter/angularjs": "^1.0.5" recently and unit tests are crawling again. Roughly 2 minutes for ~400 tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants