Improve performance of translateToContainerFullname function

[joshvfleming]

While CPU profiling our app, we noticed that the
translateToContainerFullname function was taking quite a lot of time,
and this seemed to be mostly on account of regex use. This commit replaces
the regexes with equivalent string manipulation logic. The
translateToContainerFullname disappeared from the offenders list in our
CPU profiles after the change.

Before:

After:

[stefanpenner]

@joshvfleming good catch. I guess ED was having some fun translating :P (at one point this method was rarely used)

I am glad to see an implementation change improved the speed without the need for a cach

[stefanpenner]

@joshvfleming looks like some failing tests yet, otherwise it appears like a great addition.

[rwjblue]

Looks like failing tests have to do with bad test setup (returning something as a route that isn't stamped with isRouteFactory) and changes on Ember canary.

[stefanpenner]

@joshvfleming also curious:

• which version of ember
• which version of data
• is this a prod build

[stefanpenner]

actually a quick look, indicates we likely really shouldn't be calling this method that many times.

ember-resolver/packages/ember-resolver/lib/core.js

Line 344 in 604c12d

knownForType: function(type) {

Although it is great to make it faster and we should merge this when green, calling it so many times is potentially related to a larger underlying issue. And i wouldn't be surprised if improving the calling side wouldn't have other positive side-affects.

Can you share the callstack of its invocation ? Likely expanding its entry in the profile would be sufficient.

[rwjblue]

knownForType should be called once for initial render of each test.

[joshvfleming]

Ember canary 987a593 (also latest canary, just checked)
Ember-data canary 9a85784 (same)
Prod build
Another potentially relevant data point: this is running in fastboot mode in Node, v 0.12.7

This is the stack I'm seeing:

It does seem to be a small number of really slow calls rather than an aggregation of a large number calls.

Also, I believe the test failures are unrelated to this commit -- the tests fail the same way for me with the previous commit.

[stefanpenner]

Also, I believe the test failures are unrelated to this commit -- the tests fail the same way for me with the previous commit.

ya, @rwjblue mentioned it was an issue with latest canary.

It does seem to be a small number of really slow calls rather than an aggregation of a large number calls.

interesting, so this must mean a large number of internal modules (thousands)

Can you share the number of calls 10s 100s etc.

Clearly the biggest cost was constantly creating new regexp.

[stefanpenner]

@rwjblue you seem to have a clear grasp on the test failure. Do you have time to look, or share the steps?

I would love to release a new version of the resolver shortly including @joshvfleming's work.

[stefanpenner]

@rwjblue another approach, to mitigate the need for this at all. Is to do this at compilation time, and providing some module meta-data pre-constructed. Although inlight of this PR's improvement, likely not worth it yet.

[rwjblue]

Ya, I will PR fixing the test failure to master so we can focus on this separately.

[stefanpenner]

Ya, I will PR fixing the test failure to master so we can focus on this separately.
Hide all checks

thanks

[rwjblue]

#112 should fix master

[stefanpenner]

@joshvfleming can we get a quick rebase ?

[joshvfleming]

@stefanpenner Just rebased and pushed. Regarding the call counts, I'll need to grab that from my work laptop on Monday.

Thanks @stefanpenner and @rwjblue for looking into this so quickly!

[stefanpenner]

released as v0.1.21

[rwjblue]

Thanks!