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

Roll back to polling file watching for 1.7 #6066

Merged
merged 1 commit into from
Dec 11, 2015

Conversation

zhengbli
Copy link
Contributor

This is a fix for #6016 that reverts to polling file watching for the approaching release 1.7.5
The non-polling fix is located at #6026, and will be updated later.

@vladima
Copy link
Contributor

vladima commented Dec 11, 2015

👍

zhengbli added a commit that referenced this pull request Dec 11, 2015
Roll back to polling file watching for 1.7
@zhengbli zhengbli merged commit 8c6105f into microsoft:master Dec 11, 2015
@vladima
Copy link
Contributor

vladima commented Dec 11, 2015

should this PR also go into 1.7 ? /cc @mhegazy

@zhengbli
Copy link
Contributor Author

@vladima @mhegazy I think so. Updating soon

zhengbli added a commit that referenced this pull request Dec 12, 2015
@myitcv
Copy link

myitcv commented Dec 14, 2015

@zhengbli - running v1.8.0-dev.20151214 I'm seeing ~10s delays between saving a file and recompilation in a waiting tsc --watch which is unusable... is there a configuration option I need to set now for the polling timeout or similar?

Incidentally, the previous behaviour worked fine for us 😄

$ uname -s -r -p
Linux 3.19.0-39-generic x86_64
$ node --version
v5.1.0

@zhengbli
Copy link
Contributor Author

@myitcv how many files are you watching? The current polling interval is set to 2500ms, and processes 30 files at each interval. Therefore you would experience ~10s delay only if you have more than 120 files in your project, otherwise it seems to be a bug. Currently it is not supported to be customized via a configuration file.

@myitcv
Copy link

myitcv commented Dec 14, 2015

@zhengbli - we are watching ~150 files. This doesn't feel like an acceptable rollback if the expectation is that this feature will only be used (and indeed be usable) for people who have a small number of files in their project. Rather the opposite is most likely the case - that people with lots of files use this feature for efficiency reasons during recompilation.

I haven't followed all the threads on this topic, but was it concluded this is an acceptable level of performance for --watch when compared to the problems/issues on the flip-side?

@zhengbli
Copy link
Contributor Author

@myitcv It hasn't concluded yet, as this was proposed to fix another specific issue. I admit that ~10s waiting time is not acceptable. I have changed the parameters to make the waiting time ~3s for 200-file project at #6100 while not increasing the overhead of each polling too much.

@billti
Copy link
Member

billti commented Dec 14, 2015

But we haven't changed anything here right? We just put it back to the way it was before. And we do have the more comprehensive fix coming in 1.8 (hopefully) which does away with polling anyway.

If you are supremely confident changing the settings for bigger batches and shorter intervals is super safe even on low end machines, then I'm OK with it, else I'd live with what we already shipped and wait for the 'real' fix.

@zhengbli
Copy link
Contributor Author

In 1.7 I unified the file watching used in language service and tsc, therefore it is not exactly the same with v1.6 regarding the polling mechanism. It should be safe to make the change because it didn't add much overhead overall.

@myitcv
Copy link

myitcv commented Dec 14, 2015

@billti

But we haven't changed anything here right?

For people who are using @next this change is a regression in terms of performance/behaviour. Whether it represents a regression vs. a released version of TypeScript I couldn't say because we've been using @next for months now.

I realise that @next is not to be depended upon like real releases, I'm simply flagging this as a regression within @next with rough measures of how it has impacted a project of our size (which is small in the grand scheme of things)

@zhengbli - am I right in thinking that when #6026 is merged it will effectively bring performance back (in @next) to levels pre this reversion?

@zhengbli
Copy link
Contributor Author

The change will bring the 10s waiting time down to 3s for a project of 200 files. Though another non polling fix will be merged soon that would bring the performance back to pre this reversion.

Zhengbo

From: Paul Jolly [email protected]
Sent: Dec 14, 2015 10:32 AM
To: Microsoft/TypeScript
Cc: Zhengbo Li
Subject: Re: [TypeScript] Roll back to polling file watching for 1.7 (#6066)

@billtihttps://github.com/billti

But we haven't changed anything here right?

For people who are using @next this change is a regression in terms of performance/behaviour. Whether it represents a regression vs. a released version of TypeScript I couldn't say because we've been using @next for months now.

I realise that @next is not to be depended upon like real releases, I'm simply flagging this as a regression within @next with rough measures of how it has impacted a project of our size (which is small in the grand scheme of things)

@zhengblihttps://github.com/zhengbli - am I right in thinking that when #6026#6026 is merged it will effectively bring performance back (in @next) to levels pre this reversion?

Reply to this email directly or view it on GitHubhttps://github.com//pull/6066#issuecomment-164519354.

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.

5 participants