Skip to content

Replace _mappings array with data structure #150

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 8, 2015
Merged

Replace _mappings array with data structure #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions lib/source-map/mapping-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/* -*- Mode: js; js-indent-level: 2; -*- */
/*
* Copyright 2014 Mozilla Foundation and contributors
* Licensed under the New BSD license. See LICENSE or:
* http://opensource.org/licenses/BSD-3-Clause
*/
if (typeof define !== 'function') {
var define = require('amdefine')(module, require);
}
define(function (require, exports, module) {

var util = require('./util');

/**
* Determine whether mappingB is after mappingA with respect to generated
* position.
*/
function generatedPositionAfter(mappingA, mappingB) {
// Optimized for most common case
Copy link
Contributor Author

@crisptrutski crisptrutski Jan 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To justify this unrolling, it provided a slightly over 2x speedup in my testing. Roughly corresponds to the onlyCompareGenerated flag in the util function.

Copy link
Contributor

@fitzgen fitzgen Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To justify this unrolling, it provided a slightly over 2x speedup in my testing. Roughly corresponds to the onlyCompareGenerated flag in the util function.

Can you add that as a comment then, so it is clear to future readers?

var lineA = mappingA.generatedLine;
var lineB = mappingB.generatedLine;
var columnA = mappingA.generatedColumn;
var columnB = mappingB.generatedColumn;
return lineB > lineA || lineB == lineA && columnB >= columnA ||
util.compareByGeneratedPositions(mappingA, mappingB) <= 0;
}

/**
* A data structure to provide a sorted view of accumulated mappings in a
* performance conscious manner. It trades a neglibable overhead in general
* case for a large speedup in case of mappings being added in order.
*/
function MappingList() {
this._array = [];
this._sorted = true;
// Serves as infimum
this._last = {generatedLine: -1, generatedColumn: 0};
}

/**
* Iterate through internal items. NOTE: order is NOT guaranteed.
*/
MappingList.prototype.unsortedForEach = function MappingList_forEach() {
Array.prototype.forEach.apply(this._array, arguments);
};
Copy link
Contributor

@fitzgen fitzgen Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is unused, right?

I think we should remove it -- it doesn't guarantee sorted order and I think that is a potential foot gun in the future. If we don't need it, let's remove it.

Copy link
Contributor Author

@crisptrutski crisptrutski Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, it does get used here https://github.com/crisptrutski/source-map/blob/optimistic-add-mapping/lib/source-map/source-map-generator.js#L190.

Used to make local mutations to the elements, so no cares about order. Could skip return value in wrapper to obliviate one self-harm vector, but yeah it'd be confusing with functions with non-scoped side effects.

To be honest I don't see much scope for reuse of this class though - passing in comparator as function was too much overhead so now it's really tightly coupled and just an implementation detail of the generator.

How about an ignoble this._mappings._array.forEach there and killing the wrapper method? You could consider _array to be in "friend" scope 🐐

Copy link
Contributor

@fitzgen fitzgen Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we just rename it unsortedForEach or noSortForEach so that it doesn't imply any sorting order? Should add a blurb in the doc comment about how it doesn't ensure sorting.


/**
* Add the given source mapping.
*
* @param Object aMapping
*/
MappingList.prototype.add = function MappingList_add(aMapping) {
var mapping;
if (generatedPositionAfter(this._last, aMapping)) {
this._last = aMapping;
this._array.push(aMapping);
} else {
this._sorted = false;
this._array.push(aMapping);
}
};

/**
* Returns the flat array representation of this structure.
*
* WARNING: Returns internal data without copying, for performance
*/
MappingList.prototype.toArray = function MappingList_toArray() {
if (this._sorted) {
return this._array;
} else {
// Sort runs in place
this._sorted = true;
return this._array.sort(util.compareByGeneratedPositions);
Copy link
Contributor

@fitzgen fitzgen Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to slice() like in the else branch, the return value of sort() is the same array instance:

> a = [3,45,7]
Array [ 3, 45, 7 ]
> b = a.sort()
Array [ 3, 45, 7 ]
> a == b
true
> a.pop()
7
> b
Array [ 3, 45 ]

Copy link
Contributor Author

@crisptrutski crisptrutski Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to sort in place for the amortizing behaviour, but yeah missing a slice after the sort, oops. You're probably right about finalizing the class after toArray though - in which case we then don't need the slice again (maybe I was prescient after all)

}
};

exports.MappingList = MappingList;

});
4 changes: 2 additions & 2 deletions lib/source-map/source-map-consumer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ define(function (require, exports, module) {
smc.sourceRoot);
smc.file = aSourceMap._file;

smc.__generatedMappings = aSourceMap._mappings.slice()
smc.__generatedMappings = aSourceMap._mappings.toArray()
.sort(util.compareByGeneratedPositions);
smc.__originalMappings = aSourceMap._mappings.slice()
smc.__originalMappings = aSourceMap._mappings.toArray()
.sort(util.compareByOriginalPositions);

return smc;
Expand Down
20 changes: 8 additions & 12 deletions lib/source-map/source-map-generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ define(function (require, exports, module) {
var base64VLQ = require('./base64-vlq');
var util = require('./util');
var ArraySet = require('./array-set').ArraySet;
var MappingList = require('./mapping-list').MappingList;

/**
* An instance of the SourceMapGenerator represents a source map which is
Expand All @@ -29,7 +30,7 @@ define(function (require, exports, module) {
this._sourceRoot = util.getArg(aArgs, 'sourceRoot', null);
this._sources = new ArraySet();
this._names = new ArraySet();
this._mappings = [];
this._mappings = new MappingList();
this._sourcesContents = null;
}

Expand Down Expand Up @@ -109,7 +110,7 @@ define(function (require, exports, module) {
this._names.add(name);
}

this._mappings.push({
this._mappings.add({
generatedLine: generated.line,
generatedColumn: generated.column,
originalLine: original != null && original.line,
Expand Down Expand Up @@ -186,7 +187,7 @@ define(function (require, exports, module) {
var newNames = new ArraySet();

// Find mappings for the "sourceFile"
this._mappings.forEach(function (mapping) {
this._mappings.unsortedForEach(function (mapping) {
if (mapping.source === sourceFile && mapping.originalLine != null) {
// Check if it can be mapped by the source map, then update the mapping.
var original = aSourceMapConsumer.originalPositionFor({
Expand Down Expand Up @@ -292,15 +293,10 @@ define(function (require, exports, module) {
var result = '';
var mapping;

// The mappings must be guaranteed to be in sorted order before we start
// serializing them or else the generated line numbers (which are defined
// via the ';' separators) will be all messed up. Note: it might be more
// performant to maintain the sorting as we insert them, rather than as we
// serialize them, but the big O is the same either way.
this._mappings.sort(util.compareByGeneratedPositions);
var mappings = this._mappings.toArray();

for (var i = 0, len = this._mappings.length; i < len; i++) {
mapping = this._mappings[i];
for (var i = 0, len = mappings.length; i < len; i++) {
mapping = mappings[i];

if (mapping.generatedLine !== previousGeneratedLine) {
previousGeneratedColumn = 0;
Expand All @@ -311,7 +307,7 @@ define(function (require, exports, module) {
}
else {
if (i > 0) {
if (!util.compareByGeneratedPositions(mapping, this._mappings[i - 1])) {
if (!util.compareByGeneratedPositions(mapping, mappings[i - 1])) {
continue;
}
result += ',';
Expand Down