« Back to home

Quick and Dirty JavaScript Call Site Detection

Posted on

Last week we published ember-data version 3.5.0 and made version 3.4 our first official LTS release.

Releasing an LTS means that we are able to garbage collect on minor deprecations that were scheduled to live until after that LTS was released.

For instance, the private method Store._modelForMixin was deprecated with a notice that it would be removed in or after 3.5.

deprecate(
  `_modelForMixin is private and deprecated and should
   never be used directly, use modelFor instead`,	
  false,	
  {	
    id: 'ember-data:_modelForMixin',	
    until: '3.5',	
  }	
);

Deleting Modules is Sadistic Fun

As I was going through the codebase collecting this garbage into a commit for safe, environmentally friendly disposal I found this gem, which had the attached notice:

  ## Why does this exist?!?
  
  `Ember.Map` was a private API provided by Ember (for quite some time).
  Unfortunately, ember-data made `Ember.Map` part of its public API surface via
  documentation blocks.
  
  `Ember.Map` will be deprecated and removed from Ember "soon"
  (https://github.com/emberjs/rfcs/pull/237) and we would like to confirm that
  Ember Data will work without deprecation before and after that happens.
  
  `Ember.Map` differs from native `Map` in a few ways:
  
  * `Ember.Map` has custom `copy` and `isEmpty` methods which are not present in native `Map`
  * `Ember.Map` adds a static `create` method (which simply instantiates itself with `new Ember.Map()`)
  * `Ember.Map` does not accept constructor arguments
  * `Ember.Map` does not have:
    * `@@species`
    * `@@iterator`
    * `entries`
    * `values`

  This implementation adds a deprecated backwards compatibility for:

  * `copy`
  * `isEmpty`

  ## Why is this written this way?!?

  This is needed because `Map` requires instantiation with `new` and by default
  Babel transpilation will do `superConstructor.apply(this, arguments)` which
  throws an error with native `Map`.

My heart lept, here was an entire class I could eliminate because the deprecation was complete! I live for these moments of code deletion. Had I been a tiny bit wiser, I would have paid more attention to the notice in Why is this written this way?!? before deleting the file.

On to the next one!

MapWithDefault

While clogging my clean-energy trash compactor with the custom Map implementation, I discovered an even more beautiful gem.

import Map from './map';

export default class MapWithDefault extends Map {
  constructor(options) {
    super();

    this.defaultValue = options.defaultValue;
  }

  get(key) {
    let hasValue = this.has(key);

    if (hasValue) {
      return super.get(key);
    } else {
      let defaultValue = this.defaultValue(key);
      this.set(key, defaultValue);
      return defaultValue;
    }
  }

An astute reader should notice that this custom Map extends our other custom Map. Readers more astute than myself would note that the warning about babel transpilation in our first custom Map would therefore apply here.

This is needed because `Map` requires instantiation with `new` and by default
  Babel transpilation will do `superConstructor.apply(this, arguments)` which
  throws an error with native `Map`.

I did not notice; however, and thus made the simple change of dropping the imported Map implementation so that we would now extend native Map. A short time later our "max transpilation" tests on TravisCI alerted me that Map required the use of new.

After a few experiments and some time spent Googling and reading Github threads, I realized that I had three options for how to proceed.

Option 1 - Proxy to Map

Implement the same "proxy to Map" solution for MapWithDefault that the just deleted custom Map had done. I didn't like the verbosity of this, I wanted to delete code and having these sorts of fake polyfills hanging around is exactly the sort of bloat I despise since it makes it that much harder for someone to take full advantage of transpilation targets.

Option 2 - function mapGetOrSet(map, key, defaultValue) {}

The next option was to provide some variation of a helper function that each usage of the existing MapWithDefault would use instead of relying on the subclassed Map.get method.

The trouble here was that these Map instances are often handed out to users of ember-data as public API, and while we only instantiate this class in four places, it was uncertain how many call sites for get there might be, and how many of them might rely on defaultValue.

Considering these troubles, I shoved this idea in a box and moved on to option 3.

Option 3 - instance override

The third option was to override the native get method on Map on a per-instance basis. As I mentioned before, we only instantiated this class in four places. Given the choice of instantiating 2-3 classes and proxying methods vs overriding one method on a per-instance basis, the bloat seemed smaller and performance characteristics seemed similar enough that this was the way to go.

function getDefaultValue(key) {
  let hasValue = this.has(key);

  if (hasValue) {
    return Map.get.call(this, key);
  } else {
    let defaultValue = this.defaultValue(key);
    this.set(key, defaultValue);
    return defaultValue;
  }
}

export default function createMapWithDefault(options) {
  let map = new Map();
  map.defaultValue = options.defaultValue;
  map.get = getDefaultValue;

  return map;
}

I spiked the above and converted each of the four spots we instantiated this class to methods calls.

Then I made the horrible mistake of asking Rob Jackson what he thought.

Never ask @rwjblue

I'm joking.

You should 100% always ask Rob Jackson. And if you don't ask Rob, you should ask Stefan Penner or Kris Selden to tell you why your ideas are horrible and why this garbage I was adding to the pile was worse than the garbage I was collecting.

Screenshot of conversation with Rob Jackson

I still felt that option 3 was preferrable to option 1 or option 2, but this feedback was enough to get the gears turning.

What we want is to not use this custom MapWithDefault at all. Could we.. just eliminate it? Doing so would require solving the same problem as option 2, how do we know where all the call sites are?

"get" it?

Some methods are much easier to refactor than others. For instance, a method named mySpecialMethodThatDoesX() is much easier to grep or to use code analysis tools to refactor than a method called get.

get is a very overused method name, not just in ember-data or ember but in JavaScript in general. ember-data uses get everywhere: Map, Proxy, things built over Ember.Object, OrderedSets. No tool was going to give me confidence that I'd found all the call sites, or even the right ones.

But I figured I could start by at least seeing how often this thing was used, so I refactored my implementation to tell me, figuring I could run the test suite to gauge relative importance and level of use.

function getDefaultValue(key) {
  let hasValue = this.has(key);

  if (hasValue) {
    return Map.get.call(this, key);
  } else {
    console.count('MapWithDefault.defaultValue');
    let defaultValue = this.defaultValue(key);
    this.set(key, defaultValue);
    return defaultValue;
  }
}

export default function createMapWithDefault(options) {
  console.count('MapWithDefault');
  let map = new Map();
  map.defaultValue = options.defaultValue;
  map.get = getDefaultValue;

  return map;
}

I additionally did a similar instrumentation of the linkedin.com newsfeed which I knew to use a large number of Models with many hundreds of instances as a stress test. For that I used conditional breakpoints directly on our production site.

Instrumenting linkedin.com

The results were encouraging.

  • Total Tests: 1420
  • MapWithDefault calls: 2241
  • MapWithDefault.defaultValue calls: 3074

And for Linkedin.com

  • MapWithDefault calls: 4
  • MapWithDefault.defaultValue calls: 82

Our primary use of MapWithDefault is to store information about a Model's schema (what attributes and relationships it has) in easy to access ways. This data told me that we use a couple of instances a couple of times and not much more, even when dozens of Models and hundreds of record instances are in use. If I could find the call sites, it might be painless to eliminate this custom Map implementation entirely!

don't let Rob Jackson get you down

The gears kept turning..

Right about the time Rob said this, I had an idea.

Deleting Modules is Sadistic Fun, Deleting Two Modules is an Acid Trip of Heavenly Catharsis

In my various work on schedulers and on improving the debugging experience of end users for work scheduled into queues with an async flush, I've gotten well acquainted with the ability to capture stack traces at call sites for use later.

The idea in schedulers is to capture errors thrown during a flush and rethrow them with an error stack that contains the point where work originated (where it was scheduled). This gives users a stack trace that is meaningful instead of one that ends meaninglessly wherever the queue flushed from.

Generally speaking capturing and storing traces in this way is expensive, and typically only done in non-production builds. But hey! I'm just running the test suite locally, no production semantics needed, so why not find a way to count unique call sites?

I amended my method one more time.

function getDefaultValue(key) {
  let hasValue = this.has(key);

  if (hasValue) {
    return Map.get.call(this, key);
  } else {
    console.count('MapWithDefault.defaultValue');

    let trace = new Error('Map.defaultValue used').stack;
    let lines = trace.split("\n");
    let line = lines[3];

    window.tracelines = window.tracelines || {};
    window.tracelines[line] = window.tracelines[line] || 0;
    window.tracelines[line]++;
    
    let defaultValue = this.defaultValue(key);
    this.set(key, defaultValue);
    return defaultValue;
  }
}

Now, once the test suite completed, I had the following information available stored in window.tracelines:

0: (220)  "at Class._findEmptyInternalModel (http://localhost:7357/assets/vendor.js:77297:21)"
1: (12)       "at Class._reloadRecord (http://localhost:7357/assets/vendor.js:77650:19)"
2: (1025) "at Map.forEach (<anonymous>)"
3: (1617)  "at findPossibleInverses (http://localhost:7357/assets/vendor.js:67797:7)"
4: (101)   "at Function._findInverseFor (http://localhost:7357/assets/vendor.js:68952:37)"
5: (26)     "at Array.forEach (<anonymous>)"
6: (35)     "at Proxy._findOrCreateMessages (http://localhost:7357/assets/vendor.js:66653:25)"
7: (4)        "at Proxy.unknownProperty (http://localhost:7357/assets/vendor.js:66582:25)"
8: (14)      "at Class._findRecord (http://localhost:7357/assets/vendor.js:77278:14)"
9: (4)        "at Proxy.has (http://localhost:7357/assets/vendor.js:66820:19)"
10: (11)     "at Class._scheduleFetchMany (http://localhost:7357/assets/vendor.js:77361:27)"
11: (2)       "at Class._findRecord (http://localhost:7357/assets/vendor.js:77261:21)"
12: (3)       "at Class._findRecord (http://localhost:7357/assets/vendor.js:77269:21)"

13 Total call sites, most of which were obviously within private methods and the others which looked as though they may be within private methods. I started refactoring each call site, one at a time to use the following pattern:

if (!map.has(key)) {
  map.set(key, defaultValue);
}

return map.get(key);

As I went through each location, I learned that I need not fear this becoming a breaking public API change, as each use was either entirely private API, or a public API in which the available map keys were static and accessed for the first time prior to the map instance being given to our users.

Soon, I had dropped all of our custom Map implementations completely.

The trash man commeth, and the trash man taketh away.