Skip to content
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

Memory overhead of sourcemapLocations is high #167

Closed
mihaip opened this issue Oct 29, 2019 · 3 comments · Fixed by #169
Closed

Memory overhead of sourcemapLocations is high #167

mihaip opened this issue Oct 29, 2019 · 3 comments · Fixed by #169

Comments

@mihaip
Copy link
Contributor

mihaip commented Oct 29, 2019

We were profiling the memory use of our rollup builds, and a surprising amount is due to sourcemapLocations. As far as we can tell, each AST node ends up adding two locations (https://github.com/rollup/rollup/blob/121a7f41fb88a294d9680c3fa11f4f72ebf9e9ff/src/ast/nodes/shared/Node.ts#L113-L114). Here's a screenshot from heap dump from the rollup process:

image

sourcemapLocations is an object being used as a set, if it's changed to a real Set it becomes ~4x smaller (guessing this is because it avoids the implicit stringification of object keys):

image

@Rich-Harris is this something you're open to? If so, happy to send a PR.

@paulocoghi
Copy link

paulocoghi commented Oct 30, 2019

I am not a contributor to magic-string, but I believe that if your change doesn't alter how magic-string works and all the tests pass, providing at the same time less memory overhead, surely your PR is welcome!

@kzc
Copy link
Contributor

kzc commented Nov 1, 2019

@mihaip A bit set ought to use considerably less memory than a set. Could you profile this patch?

diff --git a/src/MagicString.js b/src/MagicString.js
index 18bf1f4..e138492 100644
--- a/src/MagicString.js
+++ b/src/MagicString.js
@@ -16,4 +16,21 @@ const warned = {
 };
 
+const BitSet_BITS = 32;
+
+class BitSet {
+	constructor(arg) {
+		this._bits = arg instanceof BitSet ? arg._bits.slice() : [];
+	}
+	clone() {
+		return new BitSet(this);
+	}
+	set(n) {
+		if (n >= 0) this._bits[Math.floor(n / BitSet_BITS)] |= 1 << (n % BitSet_BITS);
+	}
+	get(n) {
+		return !!(this._bits[Math.floor(n / BitSet_BITS)] & (1 << (n % BitSet_BITS)));
+	}
+}
+
 export default class MagicString {
 	constructor(string, options = {}) {
@@ -31,5 +48,5 @@ export default class MagicString {
 			filename:              { writable: true, value: options.filename },
 			indentExclusionRanges: { writable: true, value: options.indentExclusionRanges },
-			sourcemapLocations:    { writable: true, value: {} },
+			sourcemapLocations:    { writable: true, value: new BitSet },
 			storedNames:           { writable: true, value: {} },
 			indentStr:             { writable: true, value: guessIndent(string) }
@@ -44,6 +61,6 @@ export default class MagicString {
 	}
 
-	addSourcemapLocation(char) {
-		this.sourcemapLocations[char] = true;
+	addSourcemapLocation(pos) {
+		this.sourcemapLocations.set(pos);
 	}
 
@@ -122,7 +139,5 @@ export default class MagicString {
 		}
 
-		Object.keys(this.sourcemapLocations).forEach(loc => {
-			cloned.sourcemapLocations[loc] = true;
-		});
+		cloned.sourcemapLocations = this.sourcemapLocations.clone();
 
 		cloned.intro = this.intro;
diff --git a/src/utils/Mappings.js b/src/utils/Mappings.js
index 6026200..8bf3d21 100644
--- a/src/utils/Mappings.js
+++ b/src/utils/Mappings.js
@@ -29,5 +29,5 @@ export default class Mappings {
 
 		while (originalCharIndex < chunk.end) {
-			if (this.hires || first || sourcemapLocations[originalCharIndex]) {
+			if (this.hires || first || sourcemapLocations.get(originalCharIndex)) {
 				this.rawSegments.push([this.generatedCodeColumn, sourceIndex, loc.line, loc.column]);
 			}

mihaip added a commit to mihaip/magic-string that referenced this issue Nov 19, 2019
It was using an object to store positions in the string. This meant that
those offsets were first stringified and then added as object
properties. For a 200K JS code file (as parsed by rollup), the
sourcemapLocations object was taking up 980K. Switching it to a BitSet
that uses one bit per offset makes it take up 36K instead.

Fixes Rich-Harris#167
@mihaip
Copy link
Contributor Author

mihaip commented Nov 19, 2019

Thanks @kzc, that is a lot better. Opened a PR at #169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants