Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

watcher unresponsive or tremendously slow starting with 3.12.0 #1869

Closed
mmase opened this issue Jan 26, 2017 · 8 comments
Closed

watcher unresponsive or tremendously slow starting with 3.12.0 #1869

mmase opened this issue Jan 26, 2017 · 8 comments

Comments

@mmase
Copy link

mmase commented Jan 26, 2017

Issue: After 3.9.3, watcher is virtually unresponsive or extremely slow/delayed.

Edit: I have narrowed it to https://github.com/sass/node-sass/releases/tag/v3.12.0

  • NPM version (npm -v): 3.10.8
  • Node version (node -v): 6.9.0
  • Node Process (node -p process.versions):
{ http_parser: '2.7.0',
  node: '6.9.0',
  v8: '5.1.281.84',
  uv: '1.9.1',
  zlib: '1.2.8',
  ares: '1.10.1-DEV',
  icu: '57.1',
  modules: '48',
  openssl: '1.0.2j' }
  • Node Platform (node -p process.platform): darwin
  • Node architecture (node -p process.arch): x64
  • node-sass version (node -p "require('node-sass').info"):
node-sass	3.13.1	(Wrapper)	[JavaScript]
libsass  	3.3.6	(Sass Compiler)	[C/C++]
@mmase mmase changed the title watcher unresponsive or tremendously slow after upgrade from 3.9.3 watcher unresponsive or tremendously slow starting with 3.12.0 Jan 26, 2017
@xzyfer
Copy link
Contributor

xzyfer commented Jan 27, 2017

Are you mistakenly watching your node_modules directory? We don't currently offer a way to exclude certain directories and I'd prefer not to add yet another cli flag.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 27, 2017

It's entirely possible there's a leak in the files array. I had suspected there might be at the time but I couldn't see it.

@mmase
Copy link
Author

mmase commented Jan 27, 2017

No, definitely not watching node_modules. However, we do have a fairly large sass codebase. That being said, prior to this version (3.12.0), node-sass watch triggered instantly on all of our changes. I can see if I can scrape together an example to send over.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 27, 2017

I suspect there may be a memory leak in how we're tracking the graph. I suggest doing some debugging around this array and the graph object. Their size should stay stable if you're not adding or removing files. If they're growing as you're saving then there is something wrong.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 28, 2017

I spent some time looking into this and have been unable to find any kind of leak.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 28, 2017

Ok after a lot of experimentation I was able to reproduce the issue. I was looking the watcher getting progressively slower over time which would suggest a memory leak. However the actual issue is that the watcher is slow to trigger changes on a files with lots of @imports. I've confirmed this is much slower in v3.12.0.

xzyfer added a commit to xzyfer/node-sass that referenced this issue Jan 28, 2017
the watcher for files with lots of imports. The regression was caused
by calling the slow `gaze.add` unnecessarily. We only need to call
`gaze.add` on files that aren't currently being watched.

At the time I confirmed that calling `gaze.add` in files that were
being watched wouldn't result in a leak or multiple events being
fired. I however assumed calling `gaze.add` for already watched
files would be very cheap.

Fixes sass#1869
xzyfer added a commit to xzyfer/node-sass that referenced this issue Jan 28, 2017
Watching for changes on files with lots of `@import`s had a
significant regression in responsiveness in sass#1745.

The regression was caused by calling `gaze.add` unnecessarily. We
only need to call `gaze.add` on files that aren't currently being
watched.

At the time I confirmed that calling `gaze.add` in files that were
being watched wouldn't result in a leak or multiple events being
fired. I however assumed calling `gaze.add` for already watched
files would be very cheap.

Fixes sass#1869
xzyfer added a commit to xzyfer/node-sass that referenced this issue Jan 28, 2017
Watching for changes on files with lots of `@import`s had a
significant regression in responsiveness in sass#1745.

The regression was caused by calling `gaze.add` unnecessarily. We
only need to call `gaze.add` on files that aren't currently being
watched.

At the time I confirmed that calling `gaze.add` in files that were
being watched wouldn't result in a leak or multiple events being
fired. I however assumed calling `gaze.add` for already watched
files would be very cheap.

Fixes sass#1869
@mmase
Copy link
Author

mmase commented Jan 29, 2017

Thanks @xzyfer! I've confirmed this fixes things on our end as well. Nice find.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 30, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants