handle `source-map` operations internally #3754

alexlamsl · 2020-03-28T05:46:46Z

@kzc managed to get this past the current test suite − any particular workload you have to throw at this would be welcome.

alexlamsl · 2020-03-28T05:54:00Z

test/mocha/cli.js

@@ -245,7 +245,7 @@ describe("bin/uglifyjs", function() {
            if (err) throw err;
            assert.strictEqual(stdout, [
                "var Foo=function Foo(){console.log(1+2)};new Foo;var bar=function(){function foo(bar){return bar}return foo}();",
-                "//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbInN0ZGluIiwidGVzdC9pbnB1dC9pc3N1ZS0xMzIzL3NhbXBsZS5qcyJdLCJuYW1lcyI6WyJGb28iLCJjb25zb2xlIiwibG9nIiwiYmFyIiwiZm9vIl0sIm1hcHBpbmdzIjoiQUFBQSxJQUFNQSxJQUFJLFNBQUFBLE1BQWdCQyxRQUFRQyxJQUFJLEVBQUUsSUFBTyxJQUFJRixJQ0FuRCxJQUFJRyxJQUFNLFdBQ04sU0FBU0MsSUFBS0QsS0FDVixPQUFPQSxJQUdYLE9BQU9DLElBTEQifQ==",
+                "//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbInN0ZGluIiwidGVzdC9pbnB1dC9pc3N1ZS0xMzIzL3NhbXBsZS5qcyJdLCJuYW1lcyI6WyJGb28iLCJjb25zb2xlIiwibG9nIiwiYmFyIiwiZm9vIl0sIm1hcHBpbmdzIjoiQUFBQSxJQUFNQSxJQUFJLFNBQUVBLE1BQWNDLFFBQVFDLElBQUksRUFBRSxJQUFPLElBQUlGLElDQW5ELElBQUlHLElBQU0sV0FDTixTQUFTQyxJQUFLRCxLQUNWLE9BQU9BLElBR1gsT0FBT0MsSUFMRCJ9",


While all the other source map tests are updated due to sourceRoot now appearing in the order as documented, this one is due to difference in resolving non-exact position match on overlapped mappings input source map:

{"source":"stdin","generatedLine":1,"generatedColumn":10,"originalLine":1,"originalColumn":10,"name":null} {"source":"stdin","generatedLine":1,"generatedColumn":10,"originalLine":1,"originalColumn":12,"name":null} {"source":"stdin","generatedLine":1,"generatedColumn":22,"originalLine":1,"originalColumn":23,"name":null}

When looking up for generatedColumn=13, source-map used to get stuck on originalColumn:10 while this PR will report originalColumn:11 instead.

kzc · 2020-03-28T06:48:47Z

Well done!

Like you, I don't use source maps. But you might try timing large bundles compared to the old version. You can also try using https://sokra.github.io/source-map-visualization/ - click on the custom... button to load file(s).

alexlamsl · 2020-03-28T13:13:06Z

Tested with test/benchmark.js --source-map url=inline − apparently "" counts as a valid member of names.

kzc · 2020-03-28T13:35:46Z

I ran the following command against a 5MB bundle and took the average timings of five runs using:

/usr/bin/time node-v12.13.0 bin/uglifyjs app.js --source-map -o out.js

Without PR:      2.380s
With PR:         2.438s

It appears that this PR is just a touch slower than source-map.

Edit: timings were corrected - the numbers were previously reversed.

alexlamsl · 2020-03-28T13:39:51Z

May well be my reading comprehension, but isn't (with PR) 2.380s faster than 2.438s? 😅

kzc · 2020-03-28T13:46:37Z

You read it before I had the chance to correct it. I had reversed the numbers.

kzc · 2020-03-28T14:12:17Z

The difference in timings is not significant. Keep in mind that in my 5MB example the source map creation only accounts for 0.5s of the 2.4s total run time. To get more accurate timings you should test against a larger input. Looking at the diff, there's no obvious inefficiency there. I'm guessing that the map lookups take up the majority of the time. I suppose you could run it through a profiler if you care to speed it up.

alexlamsl · 2020-03-28T14:17:18Z

Well as long as it has acceptable performance I'd rather worry more on correctness at this stage.

One thing reading the specification is that they have one-field segments − I don't understand how generatedColumn alone would be meaninful, nor has it ever happened so far in my testing.

I've tested with various combinations of flags with test/benchmark.js and the results have been identical, so I think we are good to go.

kzc · 2020-03-28T14:44:16Z

one-field segments

I think this is what magic-string calls "high resolution" source maps where the mappings are not associated with names, but merely positions. This may be related: Rich-Harris/magic-string#167

kzc · 2020-03-28T14:55:16Z

magic-string hires option: https://github.com/Rich-Harris/magic-string#sgeneratemap-options-

alexlamsl · 2020-03-28T18:11:19Z

Just read through it & they would only emits four/five-length segments as well. hires merely does it per character (that's a giant source map!)

kzc · 2020-03-28T19:35:18Z

Hmm. Then I'm stumped.

alexlamsl · 2020-03-29T11:33:21Z

According to https://coffeescript.org/annotated-source/sourcemap.html

Segments can be 1, 4, or 5 values. If just one, then it is a generated column which doesn’t match anything in the source code.

alexlamsl · 2020-03-29T11:35:36Z

May be it's for cases where new code is inserted & you want to stop that fuzzy location lookup from landing on the wrong original location.

kzc · 2020-03-29T13:03:06Z

It seems unnecessary and would bloat the source map, but that must be the reason. I wonder if any major tool creates 1 field segments.

Although uglify doesn't emit such segments, could it handle input source maps using them? i.e., ignore such mappings?

kzc · 2020-03-29T13:26:48Z

It just occurred to me that 1 field segments need not necessarily be inefficient - rather than marking every single generated character, you'd only need to mark the first generated character at the start of each generated block of characters. That would be enough information to signal to the source map consumer (typically a browser) where the previous 4/5 field source mapping ends. I always wondered why the source map mapping did not support a length - that workaround may be the reason.

alexlamsl · 2020-03-29T13:57:40Z

There are many things this format that is counter-intuitive & inefficient, so there's that 😉

I think now that I understand the single-field case we need to skip them during lookups.

kzc · 2020-03-29T14:45:38Z

I suppose the 1 field generatedColumn mappings can still be retained for lookup purposes rather than being ignored/dropped. The other fields (source, originalLine, originalColumn, name) would just be undefined or null. I'm assuming that generatedLine can be calculated on the fly.

alexlamsl · 2020-03-29T20:56:37Z

generatedLine is always there − it's from all those ;s rather than being a delta-VLQ-encoded number.

alexlamsl · 2020-04-15T11:36:32Z

While the direct output case has similar performace to source-map, the input case is measurably faster:

`uglify-js 3.8.1`

$ uglifyjs es5.js -b braces --rename -o test.js --timings
- parse: 4.360s
- rename: 3.103s
- compress: 0.000s
- scope: 0.000s
- mangle: 0.000s
- properties: 0.000s
- output: 4.113s
- total: 11.576s

$ uglifyjs es5.js -b braces --rename --source-map -o test.js --timings
- parse: 4.389s
- rename: 3.157s
- compress: 0.000s
- scope: 0.000s
- mangle: 0.000s
- properties: 0.000s
- output: 6.286s
- total: 13.832s

$ uglifyjs test.js -mc --source-map content=test.js.map -o min.js --timings
- parse: 4.402s
- rename: 3.021s
- compress: 47.614s
- scope: 1.824s
- mangle: 5.157s
- properties: 0.000s
- output: 10.759s
- total: 72.777s

`uglify-js 3.9.1`

$ uglifyjs es5.js -b braces --rename -o test.js --timings
- parse: 4.354s
- rename: 3.123s
- compress: 0.000s
- scope: 0.000s
- mangle: 0.000s
- properties: 0.000s
- output: 3.965s
- total: 11.442s

$ uglifyjs es5.js -b braces --rename --source-map -o test.js --timings
- parse: 4.431s
- rename: 3.125s
- compress: 0.000s
- scope: 0.000s
- mangle: 0.000s
- properties: 0.000s
- output: 6.137s
- total: 13.693s

$ uglifyjs test.js -mc --source-map content=test.js.map -o min.js --timings
- parse: 4.284s
- rename: 3.084s
- compress: 46.679s
- scope: 2.052s
- mangle: 5.122s
- properties: 0.000s
- output: 7.017s
- total: 68.238s

(In case it wasn't clear, the output timing includes that of source map handling.)

kzc · 2020-04-15T16:59:31Z

Good stuff.

There may also be some source map computation caching opportunities when I glanced at the code. Perhaps not worth the trouble.

Now you only need to get rid of commander to have no dependencies.

kzc · 2020-04-16T22:59:50Z

Could you rerun the timings with names=false?

alexlamsl · 2020-04-17T08:46:16Z

$ uglifyjs es5.js -b braces --rename --source-map names=false -o test.js --timings
- parse: 4.317s
- rename: 3.097s
- compress: 0.000s
- scope: 0.000s
- mangle: 0.000s
- properties: 0.000s
- output: 5.671s
- total: 13.085s

$ uglifyjs test.js -mc --source-map content=test.js.map,names=false -o min.js --timings
- parse: 4.250s
- rename: 3.048s
- compress: 44.155s
- scope: 1.829s
- mangle: 4.943s
- properties: 0.000s
- output: 6.152s
- total: 64.377s

	names=true	names=false
test.js.map	12,021,165	8,335,130
min.js.map	11,019,068	7,498,617

kzc · 2020-04-17T13:34:39Z

Not bad at all. So on average names=false is 5% faster and produces 10% smaller source maps.

In a future minor bump you might consider changing the default to names=true and see if anyone notices.

alexlamsl commented Mar 28, 2020

View reviewed changes

handle source-map operations internally

5664797

alexlamsl force-pushed the source-map branch from daaa88d to 5664797 Compare March 28, 2020 13:12

alexlamsl merged commit 827bcec into mishoo:master Mar 28, 2020

alexlamsl deleted the source-map branch March 28, 2020 14:19

Krinkle mentioned this pull request Sep 4, 2020

Reduce size of dependency graph 11ty/eleventy#1375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle `source-map` operations internally #3754

handle `source-map` operations internally #3754

alexlamsl commented Mar 28, 2020

alexlamsl Mar 28, 2020

kzc commented Mar 28, 2020

alexlamsl commented Mar 28, 2020 •

edited

kzc commented Mar 28, 2020 •

edited

alexlamsl commented Mar 28, 2020

kzc commented Mar 28, 2020

kzc commented Mar 28, 2020

alexlamsl commented Mar 28, 2020

kzc commented Mar 28, 2020

kzc commented Mar 28, 2020

alexlamsl commented Mar 28, 2020

kzc commented Mar 28, 2020

alexlamsl commented Mar 29, 2020

alexlamsl commented Mar 29, 2020

kzc commented Mar 29, 2020

kzc commented Mar 29, 2020

alexlamsl commented Mar 29, 2020 •

edited

kzc commented Mar 29, 2020

alexlamsl commented Mar 29, 2020

alexlamsl commented Apr 15, 2020

kzc commented Apr 15, 2020

kzc commented Apr 16, 2020 •

edited

alexlamsl commented Apr 17, 2020

kzc commented Apr 17, 2020

handle source-map operations internally #3754

handle source-map operations internally #3754

Conversation

alexlamsl commented Mar 28, 2020

alexlamsl Mar 28, 2020

Choose a reason for hiding this comment

kzc commented Mar 28, 2020

alexlamsl commented Mar 28, 2020 • edited

kzc commented Mar 28, 2020 • edited

alexlamsl commented Mar 28, 2020

kzc commented Mar 28, 2020

kzc commented Mar 28, 2020

alexlamsl commented Mar 28, 2020

kzc commented Mar 28, 2020

kzc commented Mar 28, 2020

alexlamsl commented Mar 28, 2020

kzc commented Mar 28, 2020

alexlamsl commented Mar 29, 2020

alexlamsl commented Mar 29, 2020

kzc commented Mar 29, 2020

kzc commented Mar 29, 2020

alexlamsl commented Mar 29, 2020 • edited

kzc commented Mar 29, 2020

alexlamsl commented Mar 29, 2020

alexlamsl commented Apr 15, 2020

uglify-js 3.8.1

uglify-js 3.9.1

kzc commented Apr 15, 2020

kzc commented Apr 16, 2020 • edited

alexlamsl commented Apr 17, 2020

kzc commented Apr 17, 2020

handle `source-map` operations internally #3754

handle `source-map` operations internally #3754

alexlamsl commented Mar 28, 2020 •

edited

kzc commented Mar 28, 2020 •

edited

alexlamsl commented Mar 29, 2020 •

edited

`uglify-js 3.8.1`

`uglify-js 3.9.1`

kzc commented Apr 16, 2020 •

edited