Opened 13 years ago

Closed 12 years ago

#1118 closed defect (fixed)

inefficient comment stripping in buildUtil.js

Reported by: tim@… Owned by: alex
Priority: high Milestone: 0.9
Component: BuildSystem Version: 0.2
Keywords: Cc:
Blocked By: Blocking:

Description

buildUtil.js has the following function:

function removeComments(contents){
	// if we get the contents of the file from Rhino, it might not be a JS
	// string, but rather a Java string, which will cause the replace() method
	// to bomb.
	contents = new String((!contents) ? "" : contents);
	// clobber all comments
	contents = contents.replace( /^(.*?)//(.*)$/mg , "$1");
	contents = contents.replace( /(
)/mg , "__DOJONEWLINE");
	contents = contents.replace( //*(.*?)*//g , "");
	return contents.replace( /__DOJONEWLINE/mg , "
");
}

It appears that the purpose of the DOJONEWLINE replacements is to get around the (.*?) regex not matching newlines. A much simpler (and faster) method is to use ([sS]*?), that is, match all whitespace and non-whitespace characters (and thereby including newlines). This results in the much more compact version of the function:

function removeComments(contents){
	// if we get the contents of the file from Rhino, it might not be a JS
	// string, but rather a Java string, which will cause the replace() method
	// to bomb.
	contents = contents ? new String(contents) : "";
	// clobber all comments
	return contents.replace( /(/*([sS]*?)*/|//(.*)$)/mg , "");
}

Apart from getting rid of the DOJONEWLINE hack, this version is also significantly faster: it saved around 3 seconds off of our compile time. (String manipulation in Javascript is *very* slow.)

Change History (2)

comment:1 Changed 13 years ago by dylan

Milestone: 0.5

comment:2 Changed 12 years ago by Adam Peller

Resolution: fixed
Status: newclosed

(In [9707]) Thanks, Tim. Shaved about a second off a base build for me. Amazingly, the year-old patch still applies. Fixes #1118

Note: See TracTickets for help on using tickets.