Thanks!

LGTM

I found a snag with this PR:

$ echo 'a,b; c,d; e,f; g,h; i,j; k;' | bin/uglifyjs -c sequences=3 -b
a, b, c, d, e, f, g, h, i, j, k;

uglify-js@2.7.5 did this by comparison:

$ echo 'a,b; c,d; e,f; g,h; i,j; k;' | bin/uglifyjs -c sequences=3 -b
a, b, c, d;
e, f;
g, h, i, j;
k;

The following patch:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -730,7 +730,10 @@ merge(Compressor.prototype, {
                 seq = [];
             };
             statements.forEach(function(stat){
-                if (stat instanceof AST_SimpleStatement && seqLength(seq) < compressor.sequences_limit) {
+                if (stat instanceof AST_SimpleStatement) {
+                    if (seqLength(seq) >= compressor.sequences_limit) {
+                        push_seq();
+                    }
                     seq.push(stat.body);
                 } else {
                     push_seq();

produces:

$ echo 'a,b; c,d; e,f; g,h; i,j; k;' | bin/uglifyjs -c sequences=3 -b
a, b, c, d;
e, f, g, h;
i, j, k;

Which I think is fine. It's better to occasionally exceed the sequences limit when joining two sequences rather than wasting CPU cycles making sequences of the exact length.

Edit: patch was corrected.

IIRC I followed the original issue, #1145, and this sequence_limit was put in place to workaround performance limitations.

So on one hand this isn't put in to be accurate hence the current (shorter) fix is sufficient. On the other hand, one could argue performance degradation might happen if I put in:

a0, a1, ... aN;
b0, b1, ... bN;

For very large N. And this isn't so contrived either, as passes=N would double the sequence length at each pass.

So may be to fully make this performance workaround to function, your suggestion makes most sense?

(I was tempted to investigate and fix the root cause for this performance degradation, but other issues got in the way 😉)

As you wrote, the sequences limit is in there for performance. The limit is a lower bound. I think the patch above is sufficient.

For very large N. And this isn't so contrived either, as passes=N would double the sequence length at each pass.

That crossed my mind but was unable to create such a case:

$ echo 'a,b,c,d,e,f; g,h,i,j,k,l; m,n,o;' | bin/uglifyjs -b -c sequences=3,passes=3
a, b, c, d, e, f;
g, h, i, j, k, l;
m, n, o;

If you can come up with a concrete example, please post it.

Side note: traversing AST_Seq is a recursive operation that wastes stack/memory/CPU. Long term it should be converted to an array. This is mentioned in the wish list: #1411 (comment)

As you stated, I missed the CHANGED-governed iteration loop in tighen_body() with my analysis.

Since I never hit that performance corner case to begin with (& I was sitting on the metro 😛), I was only speaking of theoretical possibility, and it would be hard for me to construct a real case.

So I'll stick with your suggestion of keeping with the current PR. If nothing else you've just pointed out an extra opportunity for removing AST_Return.

And since I'm waiting for all those PRs to merge before I can carry on with my current course, I might as well start reading up on AST_Seq to implement your suggestion in an upcoming PR.

The AST_Seq array change is probably a major API change that should coincide with a 3.x release.

The AST is documented at http://lisperator.net/uglifyjs/ast and some user code in the wild may depend on its structure.

@kzc fair point. I'll see if I can workaround the performance issue without changing AST_Seq then (could be an internal representation before laying it all back out as AST_Seq, for instance).

So I'll stick with your suggestion of keeping with the current PR.

The wording of that sentence is a bit ambiguous. I assume you mean you'll use the patch above?

In any case, could you please add this test case?

$ echo 'a,b; c,d; e,f; g,h; i,j; k;' | bin/uglifyjs -c sequences=3 -b

@kzc not just ambiguous, I don't think the sentence even makes sense. Sorry about that.

PTAL at the force-pushed code.

I'll see if I can workaround the performance issue without changing AST_Seq then (could be an internal representation before laying it all back out as AST_Seq, for instance).

I'm skeptical using 2 different representations of AST_Seq would lead to faster code - or maintainable code for that matter. I think it needs to be a clean API break - and probably change the AST node class name to something else to remove a potential source of confusion.

LGTM
(Looks Gooder To Me).
;-)

@kzc point taken on the potential maintenance nightmare. I'll focus my attack on the CHANGED loop and see if I can improve on the current state of affairs then.

I had a patch kicking around to remove recursion from AST_Seq traversal while retaining the car/cdr structure and it did in fact work to a large extent but some code had some implicit assumptions about order of traversal so I gave up on that experiment.

Probably not related, but I just noticed there's an assumption that AST_Seq.car cannot be AST_Seq.

Another observation: the fact that sequences are often converted to arrays and back to sequences again suggests to me that arrays would be a better internal representation. Once in array form they'd be similar to AST_Blocks holding an array of statements and more readily transformable.