[BUGFIX release] Fix classify for spaced strings

[skeate]

Resolves #13042

[rwjblue]

Will need to be [BUGFIX release] (due to LTS) once you have the failing tests fixed...

[skeate]

Tests are passing locally for me; not sure why they aren't in Travis. Please excuse the debugging.

[skeate]

Looks like it's a difference of phantomjs versions. 1.9.8 doesn't handle RegExp's test method properly.

Fortunately in this case, getting rid of the g flag works (fingers crossed), since it's looping anyway.

[rwjblue]

The fix looks good to me.

I believe the performance impact (adding a loop of regexps) to be a somewhat negligible difference due to the caching we do with these methods, but I would love @stefanpenner or @krisselden to confirm/deny.

[skeate]

One potentially better solution, if the loop is too much of performance concern, might be to add a regexp to handle the specific case of a one-character piece (which is, I believe, the only time this bug manifests).

[stefanpenner]

does ember or ember-data use classify anywhere? I don't know if its even relied upon, which leads me to believe it wont affect performance for most people.

[rwjblue]

ember-data does not use it.

uses that I am aware of:

• resolver (both globals and ember-cli) for determining resolveFooBar method name
• ember-inspector's container debug adapter (displaying descriptions of items)

[stefanpenner]

resolver (both globals and ember-cli) for determining resolveFooBar method name

ah, hmm.

resolving type should usually be a cache hit, as the number of types are relatively low.

The only way to be sure is to test it in a large app, and see how many hits/misses etc.

[rwjblue]

@stefanpenner - Ya, exactly. I think the resolver lookups would definitely be nearly all cache hits (there are only like 5 or 6 types in the system).

[mixonic]

Next step here would maybe be to recreate an app with a lot of lookup in https://github.com/eviltrout/ember-performance and confirm there isn't a regression. Or alternatively make a one-off regression check against a real app.

[homu]

☔ The latest upstream changes (presumably #13381) made this pull request unmergeable. Please resolve the merge conflicts.

[homu]

☔ The latest upstream changes (presumably #13658) made this pull request unmergeable. Please resolve the merge conflicts.

[rondale-sc]

@mixonic Created a benchmark with ember-performance eviltrout/ember-performance#107

Each test run classifies 1k random strings. Results:

master	pr/13044
770.88/per second	572.39/per second

[tomdale]

Seems like close enough performance, give one is horribly broken and the other one works.

[rwjblue]

@skeate - Would you mind rebasing one last time, then we can land this?

[skeate]

well I've gone and completely hosed this up

[skeate]

Also: Are those columns mislabeled, or am I misinterpreting, or is this PR somehow faster than what's in master?

[mixonic]

@skeate yeah it looks like the PR performs better. I expect b/c the if statement using test often is false and avoids a string allocation? Might be fun to play with further optimizations now that there is a performance PR :-)

[stefanpenner]

Seems like close enough performance, give one is horribly broken and the other one works.

We should be careful, as we have caches involved here. Real apps may blow this limits and cause some grief, as the micro benchmark path and real path may be substantially different.

We will have to be careful and do some tests in real apps. These caches have statistic tracking built in so it should be possible.

@krisselden this might be a good thing to try in one of the LI apps.

[stefanpenner]

I also think, this shouldn't be a [BUGFIX release] at the very least beta maybe canary, its possible people are depending on this behavior and fixing the output etc.

[mixonic]

@stefanpenner per the original issue #13042 this is a regression in 2.4 not present in 1.13 or 2.0. Seems pretty major, it seems unlikely any app relies on a classified string have a space 😬

Ideally still a [BUGFIX release] back to LTS I think. It is so late you can argue the point, but the buggy behavior seems broken enough to be more trolling than possibly relied upon.

[stefanpenner]

ah its a regression, i see.

[rondale-sc]

@skeate they were mislabeld. Sorry. I had meant to attach screenshots, but could not because of conference wifi.

[Serabe]

Did anyone had time to perf this? Anyway I can help?

[mixonic]

I believe this blocked on a performance regression check in a real app, per @stefanpenner's comment. Embarrassingly, this regression has been around a whole LTS cycle.

[locks]

Hi @skeate, I am sorry that we let this PR sit in limbo for so long.

We have discussed this in the last core meeting and we decided we weren't comfortable changing the behaviour of an API that has been around for so long–mea culpa.

Instead, we would like to actually deprecate this and some similar methods, and move then to an addon. This way you can use the addon if you want the current behaviour, you can use another library like lodash, or you can implement it yourself.