Load snippets lazily

[as-cii]

Our current situation on master looks like the following during startup:

Out of 3s, ~410ms are spent in loading snippets:

Although this doesn't synchronously affect Atom's initialization (i.e. window.onload), it has an heavy impact on user experience. Indeed, trying to disable the snippets package results in a more fluid and responsive startup.

The idea with this PR is to avoid doing all the work of parsing snippets upfront and to defer it to the last possible moment. In the past, other packages relied on atom.config.get to access loaded snippets: because of configuration being eager by design, we couldn't afford this anymore and we've provided a service instead for other packages to use in order to introduce laziness.

The bad news is that this will be a breaking change, whereas the good news is that snippets take ~30ms to be loaded, thus not getting in the way of other important tasks such as background tokenization, etc. Any ideas on how to handle this? I think that just :arrow_up: bumping the major version could be fine here, because the only package I have found which currently uses snippets that way is autocomplete-snippets and I am going to update it shortly.

Also, please note that I ended up not implementing a cache mechanism for parsed snippets, as the amount of parsing we do now is quite minimal and it doesn't seem to affect responsiveness on each keystroke.

/cc: @nathansobo @maxbrunsfeld @atom/feedback

[nathansobo]

Also, please note that I ended up not implementing a cache mechanism for parsed snippets, as the amount of parsing we do now is quite minimal and it doesn't seem to affect responsiveness on each keystroke.

How long do snippets take to parse? Paying this every time we expand one is a bit of a bummer, but if it's way below frame budget then it's fine.

[nathansobo]

Code looks fine to me. So long as you're sure we aren't hosing any third party packages I'd say :ship: once you have a backward-compatible update to the AC+ provider in place.

[as-cii]

How long do snippets take to parse? Paying this every time we expand one is a bit of a bummer, but if it's way below frame budget then it's fine.

The time spent in parsing snippets varies on the number of them, but I have measured it takes ~12-15ms to parse a bunch of them (n_snippets <= 10). This doesn't seem to impact the responsiveness, though, and that's why I didn't explore a caching solution. Coming back to this, however, I agree with you that not having it is somewhat a bummer, so before :ship:ping this I'll implement one.

Code looks fine to me. So long as you're sure we aren't hosing any third party packages I'd say once you have a backward-compatible update to the AC+ provider in place.

https://github.com/atom/autocomplete-snippets/pull/60 should be good to go, as CI is :green_heart: after making the code backwards-compatible. No other packages is seemingly using atom.config.get('snippets') but I'll have another :eyes: just to be 100% sure.

Thanks for the review, will ping you as soon as I make the changes.

[as-cii]

Okay, so I had another look at which packages were using snippets through atom.config and I found out that settings-view was using some atom.config internal to access all the loaded snippets. I provided a safer alternative in 9967bfc and used it in https://github.com/atom/settings-view/issues/694 in a backwards-compatible manner.

I've also implemented a simple caching mechanism which addresses the concerns brought up in https://github.com/atom/snippets/pull/182#issuecomment-154489088.

I think this is ready to be :ship:ped but it would be great to have another pair of :eyes: before doing so. Thanks! :pray:

[maxbrunsfeld]

:sparkles: This all looks most excellent to me.

[as-cii]

Published as v1.0.1, as theoretically this is a breaking change (publishing 1.0.0 gave an error, so I published a new version incrementing only the patch number).

[jeancroy]

If snippets continue to be a bottleneck there's probably room for improvement for the parsing per se. Especially the claim that we cannot deal with \ style escape in emca regex is not quite true.

That being said regex isn't especially fast for anything so maybe it doesn't matter.