Replace _mappings array with data structure

[crisptrutski]

Closes #149

Optimized for best case (called with sorted mappings), with comparable performance otherwise.

[crisptrutski]

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

[fitzgen]

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?

[lydell]

last -> list ?

[crisptrutski]

Good catch - looks like this code is untested too. Perhaps redact this unused method even?

[crisptrutski]

Let me know if I should squash the fixup commits

[crisptrutski]

Here are the benchmarks I've run so far (couldn't get markdown table to render in comment for some reason, so excuse poor legibility)

1. Data structure performance

These all use 10,000 mappings.

Push to Array only (ie. unsorted at the end)

 11,440 ops/sec ±0.49% (83 runs sampled) 

Push to Array then use .sort

 sorted input: 198 ops/sec ±1.08% (84 runs sampled) unsorted input: 191 ops/sec ±1.09% (87 runs sampled) 

Push to MappingList then request sorted array

 sorted input: 3,105 ops/sec ±0.57% (92 runs sampled) <<-- oh yeah unsorted input 185 ops/sec ±0.93% (85 runs sampled) 

2. Overall performance (use generator to produce JSON string)

Using Array for _mappings

Sorted input

10,000 @ 41.61 ops/sec ±0.95% (55 runs sampled) 100,000 @ 3.12 ops/sec ±2.14% (12 runs sampled) 1,000,000 @ 0.25 ops/sec ±2.27% (5 runs sampled) 

Unorted input

10,000 @ 40.60 ops/sec ±1.38% (54 runs sampled) 100,000 @ 2.87 ops/sec ±2.32% (12 runs sampled) 1,000,000 @ 0.21 ops/sec ±5.91% (5 runs sampled) 

Using MappingList for _mappings

Sorted input

10,000 @ 48.52 ops/sec ±0.98% (63 runs sampled) 100,000 @ 3.68 ops/sec ±2.18% (14 runs sampled) 1,000,000 @ 0.30 ops/sec ±3.89% (5 runs sampled) 

Unorted input

10,000 @ 39.27 ops/sec ±1.50% (52 runs sampled) 100,000 @ 2.83 ops/sec ±1.97% (12 runs sampled) 1,000,000 @ 0.21 ops/sec ±5.26% (5 runs sampled) 

Using MappingList for _mappings, and skipValidation on

Sorted input

10,000 @ 64.56 ops/sec ±1.08% (67 runs sampled) 100,000 @ 4.50 ops/sec ±1.69% (16 runs sampled) 1,000,000 @ 0.34 ops/sec ±13.41% (5 runs sampled) 

Unorted input

10,000 @ 50.20 ops/sec ±0.82% (65 runs sampled) 100,000 @ 3.20 ops/sec ±3.46% (12 runs sampled) 1,000,000 @ 0.23 ops/sec ±6.82% (5 runs sampled) 

[fitzgen]

Great results, thank you for the detailed benchmarking!

Looking at the code now...

[fitzgen]

Nitpick: please wrap comments at column 80

[fitzgen]

This looks great! Thank you for taking the time to match the existing style, etc. It is appreciated.

Will merge the PR once you rebase+squash and fix up the small comments I had.

As a follow up, it seems that once we call toArray on a MappingList, we never use the MappingList again. Taking advantage of this observation, we could add a release method to MappingList which invalidates the MappingList instance and returns its internal array. This skips the array allocation and the O(n) copying of items into the new array that happens when we return this._array.slice(). We would need to add runtime assertions that we don't use that MappingList instance again afterwards. Rather than adding an if at the start of every method, which would be a slow down when building up the MappingList, we could monkey patch each method with a new function that just throws an assertion failure error.

[crisptrutski]

release makes it sound like the concern is GC - should I be removing the array reference also? Not convinced this is necessary, think it made sense for the data to have the same lifespan as the generator.

How about finalize or seal? It would leave the array in place, if necessary sort in-place and set _sorted to true, and just monkey patch add.

Sound about right?

[crisptrutski]

Actually release and finalize both sounds like memory management, and seal sounds like inheritance. Going with freeze in first spin.

[fitzgen]

The lifetimes are not the same because as soon as you give out the internal array, the instance is invalidated. If we had something like rust's immutable borrows, then maybe not, but...

I like release because you are releasing ownership of the array. Maybe releaseArray?

[crisptrutski]

Actually, think we should abort this last change - it's a breaking change (see test ignore duplicate mappings. test). I'm actually fine with skipping the slice WITHOUT breaking add, because the array never escapes the generator.

[fitzgen]

releaseArrayAndInvalidate?

[crisptrutski]

Well, repeated calls to serialize the generator would reuse the _mapping instance, so more work would be required to decouple their lifetimes.

[fitzgen]

Ah good point. I guess we should just give up on the idea of enforcing safety here then and just trust SourceMapGenerator to do the right thing.

[crisptrutski]

Think all points have been addressed - let me know if you're satisfied and I'll squash 'em all tomorrow

[fitzgen]

Yup looks good, thanks! Let me know when you've squashed.

[crisptrutski]

@fitzgen squasherized on latest master

[fitzgen]

Thanks!

[crisptrutski]

Thanks for the library and thanks for accepting my self-serving amendments :)

[fitzgen]

No problem, 0.1.43 published.