webark / ember-cli-styles

4 stars 2 forks source link

Huge performance hit when using `ember-cli-styles` #18

Closed boris-petrov closed 2 years ago

boris-petrov commented 2 years ago

I finally managed to migrate my app to ember-cli-styles, Ember 4 and all the good stuff. Thanks for the hard work!

However, I noticed a horrible performance degradation. I narrowed it down to ember-cli-styles. Just adding it causes my integration tests to slow down by almost 40% (FORTY percent). Any ideas why that might be? Could it be because of this processing? I believe that happens at runtime? What else executes at runtime that might be causing such massive slowdowns?

cc @webark

webark commented 2 years ago

hmmm.. That is really the only thing I'd imagine. It is using a "debug" api (the one ember inspector uses).

We can remove that part, put then that breaks the way you are wanting to use the component, and would also break the automatic route name-spacing.

I can revisit the route namespacing, and perhaps there's a different way to do that.. But then you'd need to handle your cross component lookups a different way.

:/

webark commented 2 years ago

Thanks for vetting this BTW. I don't work on ember anymore so i can't vet it at this level.

webark commented 2 years ago

and in a real world, this would only hit on app initialization. The thing about integration tests is they do a lot of app integrations. (one for every test i think)

BryanCrotaz commented 2 years ago
 return STYLE_EXTENSIONS.reduce(function (allStyles, extention) {
    return allStyles.concat(
      owner
        .lookup('container-debug-adapter:main')
        .catalogEntriesByType(extention)
    );
  }, [])
    .filter((value, index, self) => self.indexOf(value) === index)

A recursive array.concat is doing a memory allocation for every entry. What is the filter doing? A distinct filter? This is an O(n squared) algorithm.

A Hashtable instead of an array would be much faster.

webark commented 2 years ago

@BryanCrotaz feel free to open a PR. This was something that was thrown together a while ago and haven't revisited it since. I agree that there could be a more efficient way of handling it.

webark commented 2 years ago

the registering of the "style factories" is now its own initializer, separate from the route one.

https://github.com/webark/ember-cli-styles/blob/699fb0044aeb0ac3c85cb446d7ddfd9d6a3f25b5/packages/namespace/addon/instance-initializers/register-style-factories.js#L4-L14

this could be overwritten to change from using the generateDefaultStyleNamespaceManifest to the proposed pulling in from a manually managed, or pre-generated "style namespace manifest" that is in the same structure as the current "pod-names" is in ECC.

also cleaned up the default generation a bit for those that will use this in the meantime.

https://github.com/webark/ember-cli-styles/blob/699fb0044aeb0ac3c85cb446d7ddfd9d6a3f25b5/packages/namespace/addon/utils/generate-default-style-namespace-manifest.js#L11-L23

Do you think this, along with #19 , will mitigate this performance hit? Would be nice if you could give it a try @boris-petrov (sorry for the delay in getting the latest version out).

boris-petrov commented 2 years ago

@webark - thanks for doing all this!

So, right now the runtime performance hit won't be fixed because of this initializer - it calls catalogEntriesByType for all possible extensions (as was with the previous version). So until this is changed the problem is still there.

Also, I'm not sure if you need the STYLE_EXTENSION_REGEX here at all - isn't catalogEntriesByType supposed to return only files with the requested extension? So they will always match the regex - hence the if statement is needless.

webark commented 2 years ago

It doesn't exactly do that. I'll run it without and show you why it came about.

webark commented 2 years ago

and until the build time manifest stuff is done, you could over right the initialalizer with your own (by just adding one in your app with the same name) and do just the extensions you need (cause i think you said previously this helped)

boris-petrov commented 2 years ago

Right! I didn't know one could overwrite initializers from addons! It works like a charm. And yes, with that the performance issue is (mostly) resolved. Once we get the build-time manifest it will be fully gone. :) Thank you @webark!