Opened 10 years ago

Closed 10 years ago

#8828 closed enhancement (fixed)

[ccla][patch]Shrinksafe could munge strings together

Reported by: dante Owned by: dante
Priority: high Milestone: 1.4
Component: ShrinkSafe Version: 1.3.0b2
Keywords: Cc: Adam Peller, Richard Backhouse
Blocked By: Blocking:

Description

It has been suggested in the forums that shrinksafe be enhanced to handle removing useless string concatenation as a result of newlines:

var a = "b" +
      "c" +
      "d";

should just be var a = "bcd";

but should not fail on: var a = "b" + c + "d" + "e";

should be: var a = "b"+c+"de";

patch and unit tests attached.

Attachments (5)

mungeStrings.patch (2.3 KB) - added by dante 10 years ago.
mungeStrings.2.patch (2.6 KB) - added by dante 10 years ago.
updated patch adds new unit test
changes.diff (50.3 KB) - added by dante 10 years ago.
so this isn't safe yet.
concat.patch (916 bytes) - added by Adam Peller 10 years ago.
concat strings in compress method from tokens
8828regression.patch (684 bytes) - added by Adam Peller 10 years ago.
A regression against my own patch. Granted, it looks like an edge case, but Shrinksafe ought to be safe (this could end up being a frustrating typo that shrinksafe would mask)

Download all attachments as: .zip

Change History (17)

Changed 10 years ago by dante

Attachment: mungeStrings.patch added

Changed 10 years ago by dante

Attachment: mungeStrings.2.patch added

updated patch adds new unit test

comment:1 Changed 10 years ago by Adam Peller

would it be possible/preferable to do this with tokens rather than a full pass with a regexp?

comment:2 Changed 10 years ago by Adam Peller

Component: GeneralShrinkSafe

Changed 10 years ago by dante

Attachment: changes.diff added

so this isn't safe yet.

comment:3 Changed 10 years ago by dante

var a = "+"; fails, and is an interesting issue.

Changed 10 years ago by Adam Peller

Attachment: concat.patch added

concat strings in compress method from tokens

comment:4 Changed 10 years ago by Adam Peller

Summary: Shrinksafe could munge strings together[ccla][patch]Shrinksafe could munge strings together

comment:5 Changed 10 years ago by dante

Status: newassigned

@peller - thanks for this patch, it's great! seems to work perfectly. I've got most of a new unit test going, trying to think of more cases to check. Will likely commit patch and tests this evening.

comment:6 Changed 10 years ago by dante

Resolution: fixed
Status: assignedclosed

let a sleeping trac, sleep. fixed in [19078]

comment:7 Changed 10 years ago by Adam Peller

hmm... what I did with the tokenizer is right 99% of the time, but not strictly valid.

!"a"+"b"

should yield

"falseb"

not

false

or !"ab" Perhaps we really need an AST to perform these types of optimizations?

Changed 10 years ago by Adam Peller

Attachment: 8828regression.patch added

A regression against my own patch. Granted, it looks like an edge case, but Shrinksafe ought to be safe (this could end up being a frustrating typo that shrinksafe would mask)

comment:8 Changed 10 years ago by Adam Peller

Resolution: fixed
Status: closedreopened

comment:9 Changed 10 years ago by dante

I tested:

(function(){
	var x = !"a" + "b";
})();

with YUICompressor, and the output was:

(function(){var a=!"ab";})();

so this "bug"/limitation exists there (not justifying the fail, just interesting that it exists there, too.

the overall savings of this patch against dijit-all.js (only templateString inline / pre-build) uses multi-line concatenation like this) is minimal: 60 bytes or so.

I would, however, like to offer this option as a cmd line toggle perhaps? classify this string munging as a "microoptimiztion that could potentially break code". We similarly break on "complex console.* stripping", though neek has a patch attempting to fix that.

comment:10 Changed 10 years ago by James Burke

dante: you mention this saves 60 bytes on dijit-all.js, what is the total file size with the 60 bytes? This file: http://o.aolcdn.com/dojo/1.3/dijit/dijit-all.js

is 83KB gzipped. So a 60 byte savings is like 0.07% file savings. I would say it is not worth providing the option at all, given the extra possibility of error, the extra explaining of the option, and a new feature to support.

comment:11 in reply to:  10 Changed 10 years ago by Eugene Lazutkin

Replying to jburke:

dante: you mention this saves 60 bytes on dijit-all.js, what is the total file size with the 60 bytes?

To me this optimization is about run-time cost savings: no expensive concatenations. But if it doesn't work in some cases, I think we should skip it to save our users from frustrating debugging efforts. I am saying it as someone who debugged build problems once or twice. :-(

comment:12 Changed 10 years ago by dante

Resolution: fixed
Status: reopenedclosed

(In [19120]) fixes #8828 - backing out regression causing shrinksafe patch, but leaving code commented out as the regression is a super egde case and something someone may way to manually enable for themselves.

Note: See TracTickets for help on using tickets.