New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add module discovery caching per #187 #189

Merged
merged 4 commits into from Oct 5, 2018

Conversation

Projects
None yet
2 participants
@eoneill
Contributor

eoneill commented Oct 2, 2018

This adds module discovery caching to EyeglassModules.

The cache is fairly simple and is stored per instance of EyeglassModules (which translates to per Eyeglass instance).

I tested against a fairly large code base to see the difference between per-instance vs global caching and the savings were statistically insignificant. That said, if you are creating hundreds/thousands of eyeglass instances, it might be beneficial.

Here are some metrics of time spent in discoverModules (averaged across 3 runs each):

  • no caching: 114.349s
  • per-instance caching: 34.372s
  • global caching: 37.052s

Other minor changes:

  • updated the canAccess cache to share the same SimpleCache
  • updated debug.* to only create functions if the debugger is enabled. this allows us to short-circuit some semi expensive debug message generation if we aren't going to use it.

Related: #187 #186

if (branch.name === name || branch.dependencies && branch.dependencies[name]) {
return true;
}
var cacheKey = JSON.stringify({

This comment has been minimized.

@stefanpenner

stefanpenner Oct 2, 2018

Contributor

stringify isn't stable by default, the cache key may not result in collisions due to this.

would ${origin}|${pkg.name}|${pkg.version} be sufficient, or better yet realpath of that location?

If not, maybe the following lib may be good.

@stefanpenner

This comment has been minimized.

Contributor

stefanpenner commented Oct 2, 2018

global caching: 37.052s

Do you happen to have a breakdown of the remaining cost centers?

@eoneill

This comment has been minimized.

Contributor

eoneill commented Oct 3, 2018

Previous metrics were not accurate due to double counting.

Here are updated metrics (measuring the top-level resolveModule call in EyeglassModules):

  • no caching: 78.639787s
  • per-instance caching: 35.079668s
  • global caching: 0.176994s

Raw metrics:
https://docs.google.com/spreadsheets/d/1ac0Z_hSWjf16UWlFzrTX17CKm7uq9hcaegegzSJjUmU/edit?usp=sharing

@eoneill eoneill requested a review from chriseppstein Oct 3, 2018

@eoneill eoneill merged commit 3c75d33 into sass-eyeglass:master Oct 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stefanpenner

This comment has been minimized.

Contributor

stefanpenner commented Oct 8, 2018

released as v1.6.0 🎉

stefanpenner added a commit to sass-eyeglass/broccoli-eyeglass that referenced this pull request Oct 8, 2018

@eoneill eoneill referenced this pull request Oct 30, 2018

Closed

Cache module discovery #187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment