Skip to content

clean up max_line_len#1496

Closed
alexlamsl wants to merge 1 commit intomishoo:masterfrom
alexlamsl:max_line_len
Closed

clean up max_line_len#1496
alexlamsl wants to merge 1 commit intomishoo:masterfrom
alexlamsl:max_line_len

Conversation

@alexlamsl
Copy link
Copy Markdown
Collaborator

@alexlamsl alexlamsl commented Feb 18, 2017 •
edited
Loading

Fixes #1494 (comment)

With the new warning message upon failure to trim lines properly, we should be able to detect any existing/future corner cases in the wild more easily from now on.

TODO: add tests for output and warning message

@alexlamsl alexlamsl changed the title clean up max_line_len [WIP] clean up max_line_len Feb 18, 2017
kzc
kzc reviewed Feb 18, 2017
View reviewed changes
bin/uglifyjs Outdated
beautify : BEAUTIFY ? true : false,
max_line_len : 32000,
preamble : ARGS.preamble || null,
quote_style : ARGS.quotes != null ? ARGS.quotes : 0
Copy link
Copy Markdown
Contributor

@kzc kzc Feb 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a trailing comma

Copy link
Copy Markdown
Collaborator Author

@alexlamsl alexlamsl Feb 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@alexlamsl alexlamsl closed this Feb 18, 2017
@alexlamsl alexlamsl reopened this Feb 18, 2017
@alexlamsl
clean up max_line_len
3ec5dac 
- never exceed specified limit
- otherwise warning is shown
- enabled only for final output
@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Feb 18, 2017

Tested this PR on top of #1485 with test/jetstream.js and the following options:

-mc warnings=false
-m
-b
-b bracketize
-b beautify=false
-mc passes=2,warnings=false
-mc passes=3,warnings=false
-mc unsafe,warnings=false
-mc unsafe,passes=2,warnings=false
-mc unsafe,passes=3,warnings=false
-mc collapse_vars,warnings=false
-mc collapse_vars,passes=2,warnings=false
-mc collapse_vars,passes=3,warnings=false
-mc reduce_vars,warnings=false
-mc reduce_vars,passes=2,warnings=false
-mc reduce_vars,passes=3,warnings=false
-mc collapse_vars,reduce_vars,warnings=false
-mc collapse_vars,reduce_vars,passes=2,warnings=false
-mc collapse_vars,reduce_vars,passes=3,warnings=false
-mc unsafe,collapse_vars,reduce_vars,warnings=false
-mc unsafe,collapse_vars,reduce_vars,passes=2,warnings=false
-mc unsafe,collapse_vars,reduce_vars,passes=3,warnings=false
-mc unsafe,keep_fargs=0,collapse_vars,reduce_vars,warnings=false
-mc unsafe,keep_fargs=0,collapse_vars,reduce_vars,passes=2,warnings=false
-mc unsafe,keep_fargs=0,collapse_vars,reduce_vars,passes=3,warnings=false
-mc unsafe,unsafe_comps,pure_getters,keep_fargs=0,collapse_vars,reduce_vars,warnings=false
-mc unsafe,unsafe_comps,pure_getters,keep_fargs=0,collapse_vars,reduce_vars,passes=2,warnings=false
-mc unsafe,unsafe_comps,pure_getters,keep_fargs=0,collapse_vars,reduce_vars,passes=3,warnings=false

There was one crash with -mc unsafe,keep_fargs=0,collapse_vars,reduce_vars,passes=2,warnings=false, but it was not reproducible.

@alexlamsl alexlamsl changed the title [WIP] clean up max_line_len clean up max_line_len Feb 18, 2017
@alexlamsl alexlamsl mentioned this pull request Feb 18, 2017
Closed
@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 18, 2017 •
edited
Loading

Edit: #1485 (391a25a) on Mac with -mc:

JetStream completed successfully.
real	2m58.277s

with: -mc unsafe,unsafe_comps,pure_getters,keep_fargs=0,collapse_vars,reduce_vars,passes=3,warnings=false --stats:

JetStream completed successfully.
real	5m49.514s

Why do you suppose it takes longer to run with those flags? Wouldn't the time be dominated by the benchmarks running rather than uglify?

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Feb 18, 2017
@alexlamsl
clean up max_line_len
bcae10d 
- never exceed specified limit
- otherwise warning is shown
- enabled only for final output

closes mishoo#1496
@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Feb 18, 2017

Why do you suppose it takes longer to run with those flags?

mandreel.js is a 5MB with 277,400 lines of mainly short statements. So I think we are running into inefficiencies by either AST_Seq or tighten_body().

This and a few other corner cases are worth investigating through some form of profiling. I did something along those lines by modifying DEFMETHOD when I was working on one of the PRs, but should really sit down and look at a good sample of inputs properly.

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Feb 18, 2017

With my setup, even with -mc warnings=false that file took a full minute to squeeze.

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 18, 2017

I think we are running into inefficiencies by either AST_Seq or tighten_body().

What sort of timings do you get with the AST_Sequence PR?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Feb 18, 2017

Much faster:

uglifyjs Octane2/mandreel.js -mc warnings=false --stats
Timing information (compressed 1 files):
- parse: 4.069s
- scope: 17.027s
- squeeze: 19.471s
- mangle: 0.832s
- generate: 3.840s

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Feb 18, 2017

master

uglifyjs Octane2/mandreel.js -mc warnings=false --stats
Timing information (compressed 1 files):
- parse: 4.304s
- scope: 18.319s
- squeeze: 52.408s
- mangle: 1.048s
- generate: 4.104s

#1485

uglifyjs Octane2/mandreel.js -mc warnings=false --stats
WARN: Output exceeds 32000 characters
Timing information (compressed 1 files):
- parse: 4.007s
- scope: 18.219s
- squeeze: 62.993s
- mangle: 1.045s
- generate: 4.426s

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Feb 19, 2017
@alexlamsl
clean up max_line_len
85cdee0 
- never exceed specified limit
- otherwise warning is shown
- enabled only for final output

closes mishoo#1496
@alexlamsl alexlamsl mentioned this pull request Feb 19, 2017
Merged
@alexlamsl alexlamsl closed this in 8898b8a Feb 23, 2017
@alexlamsl alexlamsl deleted the max_line_len branch February 24, 2017 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@kzc kzc kzc left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alexlamsl @kzc