stefanpenner / hash-for-dep

7 stars 11 forks source link

cache #33

Closed stefanpenner closed 7 years ago

stefanpenner commented 7 years ago

By default, cache hash-for-dep per process. I believe this is a safe change, because by default node already caches its modules this way.

rwjblue commented 7 years ago

Might take some inspiration from https://github.com/ember-cli/ember-cli/blob/master/lib/models/addon.js#L82-L110 for the instrumentation of the cache.

Basically:

heimdall.registerMonitor('addon-tree-cache', function AddonTreeCacheSchema() {
  this.hits = 0;
  this.misses = 0;
  this.adds = 0;
});

// ...snip...
Cache.prototype = {
  getItem: function(key) {
    var addonTreeCacheStats = heimdall.statsFor('addon-tree-cache');
    var cachedValue = this.__cache[key];

    if (cachedValue) {
      addonTreeCacheStats.hits++;
      treeCacheLogger.info('Cache Hit: ' + key);
      return cachedValue;
    } else {
      addonTreeCacheStats.misses++;
      treeCacheLogger.info('Cache Miss: ' + key);
      return null;
    }
  },

  setItem: function(key, value) {
    var hasValue = !!value;
    heimdall.statsFor('addon-tree-cache').adds++;
    treeCacheLogger.info('Cache Add: ' + key + ' - ' + hasValue);
    this.__cache[key] = value;
  },
  // ...snip...
}
stefanpenner commented 7 years ago

@rwjblue maybe we should extract a common instrumented cache and share it?

stefanpenner commented 7 years ago

@rwjblue I'll extract the cache, and instrument it after lunch.

rwjblue commented 7 years ago

👍 - I was thinking the same

hjdivad commented 7 years ago

@rwjblue I'll extract the cache, and instrument it after lunch.

I suspect if you extract the cache to its own (monitored) thing we may run into a double monitor registration issue as with fs-monitor.

If not, sg, but if so we may want to think about tolerating idempotent monitor registration for compatible monitors

stefanpenner commented 7 years ago

@hjdivad fixes for your feedback: https://github.com/stefanpenner/hash-for-dep/pull/34