Skip to content
This repository has been archived by the owner on Mar 23, 2024. It is now read-only.

Perf: remove regenerate dependency #861

Closed
wants to merge 1 commit into from
Closed

Conversation

efolio
Copy link
Contributor

@efolio efolio commented Dec 28, 2014

Hi,

Having big performance issues by only creating a tiny gulp task, I decided to investigate.

My example:

var gulp = require('gulp');
var jscs = require('gulp-jscs');

gulp.task('cs', function() {
  return gulp.src('index.js')
    .pipe(jscs())
  ;
});

The content of index.js (yes, it is a very big file):

angular.module('my-app', [])
  .factory('$i18nCache', function($cacheFactory) {
    return $cacheFactory('i18n');
  })
;

… so nothing out of the ordinary.

I time the execution. The result: 3s on my desktop machine… WTF?! O_O

So I profile the process to see what is going on. I stumble on this:

 [JavaScript]:
   ticks  total  nonlib   name
   1056   26.1%   58.3%  LazyCompile: dataAdd /home/etienne/work/glou/example/angular.app/node_modules/gulp-jscs/node_modules/jscs/node_modules/regenerate/regenerate.js:222
    262    6.5%   14.5%  Stub: KeyedLoadElementStub {1}
     92    2.3%    5.1%  RegExp: ^(\\/?|)([\\s\\S]*?)((?:\\.{1\,2}|[^\\/]+?|)(\\.[^.\\/]*|))(?:[\\/]*)$
     60    1.5%    3.3%  KeyedLoadIC: args_count: 0 {1}
     16    0.4%    0.9%  LazyCompile: Join native array.js:119
     13    0.3%    0.7%  LazyCompile: extend.add /home/etienne/work/glou/example/angular.app/node_modules/gulp-jscs/node_modules/jscs/node_modules/regenerate/regenerate.js:947
     12    0.3%    0.7%  Builtin: A builtin from the snapshot {1}
     10    0.2%    0.6%  Stub: CEntryStub
     10    0.2%    0.6%  RegExp: ^(\\/?|)([\\s\\S]*?)((?:\\.{1\,2}|[^\\/]+?|)(\\.[^.\\/]*|))(?:[\\/]*)$ {1}
      6    0.1%    0.3%  Stub: SubStringStub
      6    0.1%    0.3%  Stub: CompareICStub

etc…

1056 ticks on the library: regenerate!
58.3% of total user execution!

As I look into the only 2 rules using regenerate, maybe my understanding is wrong, but it seems that regenerate is unnecessary as the proper regexp are shipped along with unicode

The fun thing? This tiny little patch improves the overall performance of JSCS by a factor > 200 :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 673fdf4 on efolio:master into 5344ad7 on jscs-dev:master.

@qfox
Copy link
Member

qfox commented Dec 28, 2014

Wow ;-D

@mikesherov
Copy link
Contributor

👏

@mdevils
Copy link
Member

mdevils commented Dec 28, 2014

😮

@indexzero
Copy link
Contributor

@mrjoelkemp
Copy link
Member

LGTM. Great work. What did you use to profile the code?

@efolio
Copy link
Contributor Author

efolio commented Dec 29, 2014

I used node-tick-processor.

@mrjoelkemp
Copy link
Member

Landed! Thanks again @efolio.

@AlexanderZeilmann
Copy link
Contributor

Ahhh performance, thats one detail I didn't speed attention when I wrote this code a week ago.

Perhaps, monitoring the performance is something we could look into in the future...

So thanks @efolio for fixing my code :-)

@efolio
Copy link
Contributor Author

efolio commented Dec 31, 2014

You're welcome :)

@mathiasbynens
Copy link
Contributor

For future reference, regenerate is supposed to be used as part of a build script, not at runtime!

@AlexanderZeilmann AlexanderZeilmann mentioned this pull request Mar 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants