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

[CLOSED] [PoC] Use chokidar instead of the fsevents/fsevents_win combo #10480

Open
core-ai-bot opened this issue Aug 30, 2021 · 14 comments
Open

Comments

@core-ai-bot
Copy link
Member

Issue by ficristo
Wednesday Feb 03, 2016 at 15:13 GMT
Originally opened as adobe/brackets#12190


This change still cause significant cpu usage.
Only looked at the Activity Monitor on OSX or the Task Manager on Windows.
See #12187


ficristo included the following code: https://github.com/adobe/brackets/pull/12190/commits

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Feb 03, 2016 at 15:14 GMT


/cc@MiguelCastillo

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Wednesday Feb 03, 2016 at 15:34 GMT


@ficristo Ha! build failed because

data: {"message":"API rate limit exceeded for 52.22.60.255. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://developer.github.com/v3/#rate-limiting"}

Maybe trigger the build again and see what happens.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Thursday Feb 04, 2016 at 13:21 GMT


I had totally missed the depth parameter. Now it's a bit better.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Sunday Feb 07, 2016 at 10:10 GMT


Actually it seems to me that the file watcher is recursive on Windows and OSX.
So I removed the depth parameter and added the ignored one.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Saturday Feb 13, 2016 at 12:17 GMT


@petetnt Could you try your extension https://github.com/petetnt/corrupt-brackets-history with this PR?
I was able to run it once without errors, but running again it failed.
It's not urgent, but I'm curios if this could at least help.
Be aware that, expecially on startup, this is CPU intensive.

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Saturday Feb 13, 2016 at 14:29 GMT


@ficristo Tested it, first corruption occurred on the 116th try 😭

Otherwise I had no issues.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Saturday Feb 13, 2016 at 15:13 GMT


@ficristo let me take a step back. File watching is a very critical path piece functionality that we cannot regress on. So, it is important that we understand the impact of this proposed changes.

Do you think you can help us understand:

  1. Differences in behavior between what we have and the new library.
  2. Performance differences.
  3. Platform support.

These three questions are very important before we continue going down this path. Thank you so much for understanding why I am being very cautious about these changes :)

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Saturday Feb 13, 2016 at 16:11 GMT


@MiguelCastillo This was an intersting thing to explore for me. I don't mind if it takes it's time.
The situations for what I've understood is as follows.

Currently:

  • OSX: an old version of fsevents, customized to work for 32 bit OS (recursive)
  • Windows: a custom version of fsevents (recursive)
  • Linux: native node api (non recursive)

With chokidar:

  • OSX: a more up to date version of fsevents, 64 bit OS (recursive)
  • Windows: native node api (recursive)
  • Linux: native node api (recursive)

This probably will require an update to CEF where only OSX 64 bit is supported.
The initial CPU usage is a bit high with chokidar, probably too much. There is some effort upstream to improve it.
That's what I learned so far.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Saturday Feb 13, 2016 at 16:35 GMT


@ficristo Thats an awesome set of information. I think it makes sense to gather some metrics to understand how we will be affected. Updating CEF is a PITA. We should explore if we have any plans to update soon.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Saturday May 14, 2016 at 16:45 GMT


I've tried to add the preferences for the paths to ignore but I seems to have encountered some circular reference problems...

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Wednesday Aug 03, 2016 at 07:23 GMT


@ficristo I've build shell using adobe/brackets-shell#543 and used this PR as a base to run tests. I can see some fs watching logged to the console but running the tests fails:

image

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Aug 03, 2016 at 17:51 GMT


@zaggino I fixed the tests.
I'm still not familiar enough with the Brackets code for file watching so if you want to make any changes go ahead.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Wednesday Aug 03, 2016 at 21:02 GMT


Thanks@ficristo, will continue testing this today.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Thursday Aug 04, 2016 at 08:52 GMT


Replaced by adobe/brackets#12647.

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

1 participant