array-set has terrible memory usage

[MagicDuck]

We have a large project that is build using webpack, which uses source-map to generate the sourcemaps.
There are issues with performance of the phase that generates the sourcemaps so we did some profiling. One thing we noticed right off the bat is the sharp sawtooth shape of the memory usage graph of the node process. This basically means that a lot of the time is spent garbage collecting. Digging deeper, we profiled using the google chrome dev tools and noticed that one of the things allocating a lot of memory is array-set. It was using gobs of memory by making large source strings keys into an object which causes string copies. We monkey patched it on our side, but it probably makes sense to change this class to use some modern javascript Set/Map data structure so everybody gets the benefit.
Here's the monkey patch we use:

var ArraySet = require("webpack-core/node_modules/source-map/lib/source-map/array-set").ArraySet;

/**
 * the backing map
 * use a map to check uniqueness instead of an object's keys
 * @return {Map}
 */
ArraySet.prototype._map = function ArraySet__map() {
    if (!this._theMap) {
        this._theMap = new Map();
    }

    return this._theMap;
};

/**
 * Return how many unique items are in this ArraySet. If duplicates have been
 * added, than those do not count towards the size.
 *
 * @returns Number
 */
ArraySet.prototype.size = function ArraySet_size() {
    return this._map().size;
};

/**
 * Add the given string to this set.
 *
 * @param String aStr
 */
ArraySet.prototype.add = function ArraySet_add(aStr, aAllowDuplicates) {
    var isDuplicate = this.has(aStr);
    var idx = this._array.length;
    if (!isDuplicate || aAllowDuplicates) {
        this._array.push(aStr);
    }
    if (!isDuplicate) {
        this._map().set(aStr, idx);
    }
};

/**
 * Is the given string a member of this set?
 *
 * @param String aStr
 */
ArraySet.prototype.has = function ArraySet_has(aStr) {
    return this._map().has(aStr);
};

/**
 * What is the index of the given string in the array?
 *
 * @param String aStr
 */
ArraySet.prototype.indexOf = function ArraySet_indexOf(aStr) {
    var idx = this._map().get(aStr);
    if (idx >= 0) {
        return idx;
    }
    throw new Error('"' + aStr + '" is not in the set.');
};

[tromey]

We had a brief discussion of this on irc. I believe we're still reluctant to require anything after ES5; but on the other hand, feature detection would be ok. So if you wanted to submit this as a PR that used Map when it was available, and the existing code when it was not, then that would be alright.

[MagicDuck]

Sure, but curious why the reluctance given nodejs 4 is the minimum supported?

[tromey]

Sure, but curious why the reluctance given nodejs 4 is the minimum supported?

I don't know the answer, sorry. I'm just learning about this project. Is 4 really the minimum? All I saw was "node": ">=0.10.0" in package.json.

[MagicDuck]

4 is the lowest version on the nodejs website...

[MagicDuck]

Maybe you need to up your package.json node version. projects like webpack are already operating on the assumption of es6. Webpack is upgrading its entire codebase to use classes, etc.

[MagicDuck]

PR is here: #260
(It would still be good to understand why we can't just assume Map as it would keep the code cleaner. Even IE11 supports Map...)

[tromey]

It would still be good to understand why we can't just assume Map as it would keep the code cleaner.

Yeah, that would be good. I will try again to find out.

[tromey]

I will try again to find out.

Given the recent test failures with the old version of node, I think it is fine to just move on to the future; that is, assume Map and do whatever other cleanups seem reasonable.

[edmorley]

Hi! This was fixed on master as part of #336, however many projects can't update to source-maps 0.7.x due to node 6 not being supported and no sync API. As such it would be great if we could backport the fix to 0.6.x to help everyone using webpack etc. Would such a PR be accepted?

[edmorley]

Ah ignore that - the PR that auto-closed this wasn't the one that originally fixed the issue. This was fixed originally in #260, which made it into 0.5.7. Sorry for the noise!