change memoize to allow separate caches per ember-cli project

[lennyburdette]

I encountered the need for this change when I tried writing a test for
an addon that interacts with ember-engines. The cache holds on to
broccoli trees, including their location on disk. I was using
broccoli-test-helper which creates temp directories for each
test. Ember-engines would continue to refer to the previous test's
temp directories and fail in confusing ways. After this change, each
addon instance gets its own caches.

cc @rwjblue @stefanpenner

[lennyburdette]

I updated the PR to hide the implementation details in the memoize module instead of making engine-addon care about tracking the cache.

[lennyburdette]

aaaand I changed the implementation once more to use a WeakMap under the hood. Now memoize is automatically a per-instance cache with no other APIs exposed. PTAL!

[stefanpenner]

thanks, i'll review this shortly. If you haven't heard from by by mid-day tomorrow, please don't hesitate to bug me.

[stefanpenner]

Although I do understand this causes development of add-on ergonomic issues, I am not able to reconcile this with the potential build time performance loss. At-least not without some further thought/analysis.

Engines currently have an unfortunate build time performance impact, and that is if they or their trees are included more then once, duplicate work really slows everything down. But within 1 build instance, that duplicate tree cannot produce different results.

I believe the problem you are running into, is not that the cache is per plugin instance (too small a scope), but rather that the cache is per processes (to large a scope). Instead, it sort of feels like that cache wants to be per build instances. Does that sound reasonable?

Now, I need to think if we can even easily create such a cache from this level...

---

I'll also try and do some local tests as well, investigating if this is as big of an issue as I think it may be.

[lennyburdette]

I believe the problem you are running into, is not that the cache is per plugin instance (too small a scope), but rather that the cache is per processes (to large a scope). Instead, it sort of feels like that cache wants to be per build instances. Does that sound reasonable?

This is exactly right.

It's not clear to me why a single build would create multiple instances of an EngineAddon class. Is it just the case of including an engine more than once in a single build (e.g. engine C being a dependency of engine A and engine B)? Or is it more nuanced than that?

[lennyburdette]

All I really want to do is write a test for ember-intl that proves that I've fixed an engines-related bug, so if you've got other solutions for me, I'm open to that too! 😄

[stefanpenner]

(e.g. engine C being a dependency of engine A and engine B)?

I believe so.

I have a really big engine based app here, I'll add some instrumentation tomorrow and report that. That should answer our question.

I believe some of these memoizes don't have much value, but haven't had the time to come up with sufficient evidence. This may be a good opportunity.

[lennyburdette]

@stefanpenner did you get a chance to run some perf tests today? Or come up with any other ideas?

[lennyburdette]

@stefanpenner I realized that we could set up a new cache per ember-cli/lib/models/project instance. That should get us the granularity we want!

[lennyburdette]

@stefanpenner one more change: this now falls back to the original behavior (a single global cache) if the method receiver does not have a project property. Seems a bit safer in terms of performance. Will you have a chance to look at this soon?

[stefanpenner]

@stefanpenner did you get a chance to run some perf tests today? Or come up with any other ideas?

Hey @lennyburdette sorry about that, I started doing the perf work and realized babel parallel builds where broken, which lead me down the path to fix parallel babel builds.

[lennyburdette]

@stefanpenner thanks for the update! That does sound more important to fix. 😁

[stefanpenner]

Also noticed, not all tests where running correctly: #554

Don't worry, I am slowly making way to this PR. (it is important to me as well, sorry for the delay)

[lennyburdette]

@stefanpenner anything I can do to push this along?

[villander]

@stefanpenner can you merge this, please? All tests are green now ; )