Replace mergeSort with Array.prototype.sort
#355
Conversation
|
Uglify switched from the built-in sort to a hand-rolled merge sort in 8e82d8d. I believe this was probably done because the built-in sort was unstable. It's only since this January that the built-in sort is required to be a stable sort.
|
|
Sorry, I didn't get what is actually wrong with |
|
It's still just a guess, but I can't think of any other reason why they would have switched to a hand-rolled merge sort. A stable sorting algorithm ensures the output is deterministic. That's important for a minifier: running Terser on the same input code with the same options should always result in the same output code. If Terser used an unstable sort algorithm for sorting mangling characters by their frequency, then the mangled names could change in different runs. Even if you're using an unstable sort, it's very hard to actually get a different output. Often, it just happens to return the same output as a stable sort would. That's made even more difficult by the fact that V8 uses an adaptive sorting algorithm: for small inputs, it uses insertion sort which is stable. I suspect that's why all tests are still passing on this branch: even though Node 6, 8 and 10 use an unstable sort, we have no tests with sufficiently large, carefully crafted input code that could result in different output code. |
| const freq = new Map(); | ||
|
|
||
| const digits = init("0123456789"); | ||
| const leading = init("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ$_"); |
There was a problem hiding this comment.
sort method only used for two arrays above which have 10 and 54 elements
There was a problem hiding this comment.
It looks like the threshold between insert sort (stable) and quick sort (unstable) is 10 elements. So digits would always use insertion sort, but leading wouldn't. 😕
|
Looks like Array sort stability is now part of the spec. But it's only on node 12+ @MattiasBuelens has mentioned node 11, but we don't need to check because node 10 will become EOL after node 11 does. |
Good point! Indeed, only even-numbered releases become LTS. 👍 Still, it's way too early to drop support for Node versions <= 10. We would have to wait until Node 10 becomes EOL, which is still more than a year away (currently scheduled for April 2021). |
|
hmmm.. according to nodejs release cycle 6, 7, 9 and 11 alread EOL, node v8 one day left, I think we can drop node 6-9 support already but it really early to drop support for 10 since it will be EOL at April 2021 :)
|
b95514a to
e140ce9
Compare
|
Can this be merged now since node v10 has reached EOL? |
|
Correct, Node 10 is now EOL. However, Terser 5 still supports Node 10 or higher, see package.json and CHANGELOG.md. I agree that Terser should drop support for EOL versions of Node, but that will require a major version bump. That's a decision for the maintainers to make. 🙂 |
|
I think I'm still not happy to merge even with node 10 far out of EOL because that would rely on non-spec behaviour. Terser also runs on the web, and that means it can run on non-v8 engines. However I am happy to get rid of a bespoke sort implementation if we can replace it with another stable sort such as lodash's sortBy. |
I might be misunderstanding you here, but |
|
Interesting @ehoogeveen-medweb! I wasn't aware of this. |
No description provided.