Really optimise activation speed

[vitch]

What Changed & Why

Building on #119 and trying to fix #120 this PR introduces a couple of new config options for this plugin:

• fetchOnlyRelevantRevisions - If set then this changes the behaviour of fetchInitialRevisions so that it only continues hitting ListObjects until it has found the currently active revision. This is done on the assumption that the main use-case for fetchInitialRevisions is to be able to build other plugins which record the previous and current version when you activate
• disableFetchRevisions - if set then this short-circuits fetchRevisions within this plugin. This is because fetchRevisions is called after activate and I'm not sure what the list of revisions is needed for (see related conversation in #120). Not loading it can speed things up considerably.

If I use this branch and set both flags for one of our apps and environments (which has ~60,000 deployments in S3) then the activate command completes in 139 seconds on my computer (down from 590s on #119 and 835s prior to that - an overall improvement of over 80%).

The way to get quicker than this is to track the currently active version more explicitly somewhere so we don't have to loop over S3 at all - at that point it feels like maybe it is a different plugin though...

Related issues

#120

PR Checklist

I'm happy to do the below stuff but thought it was worth submitting this for discussion at this point... How do people feel about the changes? Do they make sense? Would they be useful to anyone other than us? Any better names for the config options?

• [ ] Add tests
• [ ] Add documentation
• [ ] Prefix documentation-only commits with [DOC]

People

All maintainers? :)

[lukemelia]

Assuming 1) these new flags default to false and 2) the new code paths have decent test coverage to prevent regressions, I'm supportive of these changes.

[vitch]

Cool - I'll see if I can put together some test coverage...

I'm wondering about another flag (maybe disableFetchInitialRevisions although I'm open to input on naming for all of them) which completely short circuits fetchInitialRevisions. In our usage we can potentially get the information from the last active revision from elsewhere (our audit log) so we don't need to do any ListObject at all... Thoughts?

[lukemelia]

Here's another out-of-the-box idea: instead of these flags, we allow configuration of a fetchInitialRevisionsFunc that is used instead of the included implementation. Then you could customize this to use your "cache" or make it a no-op, and other people could potentially adapt it as they see fit. WDYT?

[vitch]

Interesting idea... I guess we'd want a fetchRevisionsFunc as well. And we would want the base one to accept until like on this PR (otherwise people would need to recreate that logic themselves).

At that point I guess rather than the config funcs I could just make an ember-cli-deploy-s3-index-optimised which extended this one and overwrote those functions with the behaviour I wanted. Which would be completely opt-in... And the majority of the code would still live in this package...

Thoughts?

[lukemelia]

At that point I guess rather than the config funcs I could just make an ember-cli-deploy-s3-index-optimised which extended this one and overwrote those functions with the behaviour I wanted. Which would be completely opt-in... And the majority of the code would still live in this package...

That's certainly an option that is open to you. Personally, I think the cost of maintenance would be markedly lower if you just needed to ensure that your configured functions continue to work as desired rather than deal with general dependency updates and tracking potential superclass changes.

[vitch]

I was thinking of it more from the point of view of purity of this addon... It felt a bit like the start of a slippery slope to "too many options" adding config for the choice of overriding two of the core functions...

I'm happy to add it here and see how it looks?

[lukemelia]

Go ahead and try it. I would describe ember-cli-deploy's general philosophy in this area as "purity in core, pragmatism in plugins"

[vitch]

Linked a small optimisation and a new approach above.

~~One thing I've just realised is that there is no way from the context to find out which command was run (e.g. ember deploy:list or ember deploy:activate) - unless I'm missing something? With this limitation, it means I can't configure anything such that ember deploy:list still works while still optimising for the upload / activation use cases?~~

Ignore that - I just remembered that I did it via an environment variable which is read in the config for the previous implementation...