Enable caching for ESLint #8296

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
8 participants
@Trott
Member

Trott commented Aug 27, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools

Description of change

This PR is in 3 separate commits:

  • Update ESLint to 3.4.0 (which includes a bugfix for the ESLint cache that we strictly don't need but are probably better off having than not having)
  • Adding the .eslintcache file to .gitignore
  • Enabling caching (and disabling the parallelizing script) on the non-CI lint task in Makefile and vcbuild.bat (Oh, hey, can someone on Windows grab this branch and test the vcbuild.bat change to the jslint task?)

Here's my crude benchmarks:

  • Running make jslint the first time (no cache) with this change: 11 seconds or so
  • Running make jslint on current master: 8 seconds or so
  • Running make jslint subsequent times (populated cache) with this change: 1 second

Here's my thinking:

make jslint is something that gets done again and again in a dev environment. It's worth an extra 3 seconds or so on the first run if it shaves 7 seconds or so on subsequent runs.

However, in CI, it just gets run once. So the current parallelizing script is the faster option on CI.

It is possible to enable caching in the parallelizing script, but I'm not sure it's worth the complexity. That can be a subsequent enhancement if it is highly desirable. It's possible (I think) that it will not be measurably better than one job with one cache. Would have to be measured, of course.

/cc @mscdex

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Aug 27, 2016

Contributor

How does the cache work? Especially, how does it detect changes to files? Does it just compare mtime?

Contributor

mscdex commented Aug 27, 2016

How does the cache work? Especially, how does it detect changes to files? Does it just compare mtime?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 27, 2016

Member

mtime and size of the file

It also stores the hash of the config file for the last time that particular file was linted. That means there is a pitfall if you change your .eslintrc file, you've invalidated your entire cache and it will have to be recreated on the next run. But I suspect that those of us who tweak the .eslintrc file with any frequency are a tiny minority of contributors...

Member

Trott commented Aug 27, 2016

mtime and size of the file

It also stores the hash of the config file for the last time that particular file was linted. That means there is a pitfall if you change your .eslintrc file, you've invalidated your entire cache and it will have to be recreated on the next run. But I suspect that those of us who tweak the .eslintrc file with any frequency are a tiny minority of contributors...

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 27, 2016

Member

If you want more info on how it determines if a file has changed:

Member

Trott commented Aug 27, 2016

If you want more info on how it determines if a file has changed:

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 27, 2016

Member

The main challenge with using caching in the parallelized script is that each worker would need its own cache file (cacheLocation property in the config object). Obviously possible, and really easy if (for example) there is a one-to-one relationship between directory and worker. (That is, if one worker gets everything in test and another gets everything in lib, then put the cache files in test/.eslintcache and lib/.eslintcache respectively.)

Member

Trott commented Aug 27, 2016

The main challenge with using caching in the parallelized script is that each worker would need its own cache file (cacheLocation property in the config object). Obviously possible, and really easy if (for example) there is a one-to-one relationship between directory and worker. (That is, if one worker gets everything in test and another gets everything in lib, then put the cache files in test/.eslintcache and lib/.eslintcache respectively.)

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 27, 2016

Contributor

I'd squash the last two commits together, but LGTM. Speeding up the linter is 😍

Contributor

cjihrig commented Aug 27, 2016

I'd squash the last two commits together, but LGTM. Speeding up the linter is 😍

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Aug 27, 2016

Contributor

Will the cache be invalidated when a new ESLint version is used?

Contributor

silverwind commented Aug 27, 2016

Will the cache be invalidated when a new ESLint version is used?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 27, 2016

Member

Rubber stamp LGTM.

Member

jasnell commented Aug 27, 2016

Rubber stamp LGTM.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 27, 2016

Member

Will the cache be invalidated when a new ESLint version is used?

Yes.

Member

Trott commented Aug 27, 2016

Will the cache be invalidated when a new ESLint version is used?

Yes.

@Trott

This comment has been minimized.

Show comment
Hide comment
@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 28, 2016

Member

Looks fine in windows, LGTM

Member

benjamingr commented Aug 28, 2016

Looks fine in windows, LGTM

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 28, 2016

Member

(CI is green)

Member

Trott commented Aug 28, 2016

(CI is green)

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Aug 28, 2016

Contributor

LGTM, maybe you can squash the second and third commit as mentioned.

Contributor

silverwind commented Aug 28, 2016

LGTM, maybe you can squash the second and third commit as mentioned.

Trott added a commit to Trott/io.js that referenced this pull request Aug 30, 2016

tools: update ESLint to 3.4.0
PR-URL: nodejs#8296
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

Trott added a commit to Trott/io.js that referenced this pull request Aug 30, 2016

tools: enable caching for jslint task
PR-URL: nodejs#8296
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 30, 2016

Member

Landed in 2011158 and 5a7a6d9

Member

Trott commented Aug 30, 2016

Landed in 2011158 and 5a7a6d9

@Trott Trott closed this Aug 30, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

tools: update ESLint to 3.4.0
PR-URL: nodejs#8296
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

tools: enable caching for jslint task
PR-URL: nodejs#8296
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

tools: update ESLint to 3.4.0
PR-URL: #8296
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

tools: enable caching for jslint task
PR-URL: #8296
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

@Trott do you want to backport these linter changes?

Member

MylesBorins commented Sep 30, 2016

@Trott do you want to backport these linter changes?

Trott added a commit to Trott/io.js that referenced this pull request Oct 1, 2016

tools: update ESLint to 3.4.0
PR-URL: nodejs#8296
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

Trott added a commit to Trott/io.js that referenced this pull request Oct 1, 2016

tools: enable caching for jslint task
PR-URL: nodejs#8296
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Oct 1, 2016

Member

@thealphanerd It looks like we've skipped a number of ESLint updates on v4.x at this point. My inclination would be to not start now, if there's a lot of annoyance involved and not much benefit. Fine by you if we mark this as dont-land and revisit if it becomes necessary? (We can always do a separate update to whatever ESLint version just for v4.x if we want to.)

Member

Trott commented Oct 1, 2016

@thealphanerd It looks like we've skipped a number of ESLint updates on v4.x at this point. My inclination would be to not start now, if there's a lot of annoyance involved and not much benefit. Fine by you if we mark this as dont-land and revisit if it becomes necessary? (We can always do a separate update to whatever ESLint version just for v4.x if we want to.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment