8000 use `watch-project` rather than deprecated `watch` · Issue #389 · wincent/command-t · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

use watch-project rather than deprecated watch #389

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.

Sign up for GitHub

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

Jump to bottom
Closed
mr-salty opened this issue Jun 16, 2022 · 3 comments
Closed

use watch-project rather than deprecated watch #389

mr-salty opened this issue Jun 16, 2022 · 3 comments

Comments

@mr-salty
Copy link
mr-salty commented Jun 16, 2022 •
edited
Loading

watch is deprecated per the watchman docs: https://facebook.github.io/watchman/docs/cmd/watch.html
it should be replaced with watch-project: https://facebook.github.io/watchman/docs/cmd/watch-project.html

The latter has the big advantage of not creating redundant watches for subdirectories. When I looked at watchman watch-list on my machine I had ~20 watches, all of which were subdirectories of my home directory. The daemon was using ~1.6GB of memory (I have about 1.5M files in my homedir).

I did watchman watch-del-all, killed the daemon, then did watchman watch-project $HOME, the current memory usage is 571MB.

The downside is you have to change how you do queries, you have to supply the "real" root returned from watch-project and put the subdirectory in the expression. It seems to me that watchman could easily split the path itself to allow existing queries to continue to work.

So, for instance:

$ watchman watch-project $HOME
{
    "version": "2022.06.13.00",
    "watch": "/Users/todd",
    "watcher": "fsevents"
}

$ watchman watch-project $HOME/subdir
{
    "version": "2022.06.13.00",
    "relative_path": "subdir",
    "watch": "/Users/todd",
    "watcher": "fsevents"
}

then, instead of using this query: ["query", "/Users/todd/subdir", { "expression": ["type", "f"], "fields": ["name"]}]
we have to do ["query", "/Users/todd", { "expression": ["allof", ["dirname", "subdir"], ["type", "f"]], "fields": ["name"]}]
and, annoyingly, strip subdir/ from the results.
@mr-salty
Copy link
Author

Here's a proof of concept I did, it's hacky but it works - I can clean it up and submit it as a PR:
Do we have to worry about supporting old versions of watchman that don't support watch-project?

diff --git a/ruby/command-t/lib/command-t/scanner/file_scanner/watchman_file_scanner.rb b/ruby/command-t/lib/command-t/scanner/file_scanner/watchman_file_scanner.rb
index 60f412a..7bddbf1 100644
--- a/ruby/command-t/lib/command-t/scanner/file_scanner/watchman_file_scanner.rb
+++ b/ruby/command-t/lib/command-t/scanner/file_scanner/watchman_file_scanner.rb
@@ -25,24 +25,27 @@ module CommandT
 
           UNIXSocket.open(sockname) do |socket|
             root = Pathname.new(@path).realpath.to_s
-            roots = Watchman::Utils.query(['watch-list'], socket)['roots']
-            if !roots.include?(root)
+#            roots = Watchman::Utils.query(['watch-list'], socket)['roots']
+
+            if true # !roots.include?(root)
               # this path isn't being watched yet; try to set up watch
-              result = Watchman::Utils.query(['watch', root], socket)
+              result = Watchman::Utils.query(['watch-project', root], socket)
 
               # root_restrict_files setting may prevent Watchman from working
               # or enforce_root_files/root_files (>= version 3.1)
-              extract_value(result)
+              watch_root = extract_value(result, 'watch')
+              relative_path = extract_value(result, 'relative_path') if result['relative_path']
             end
 
-            query = ['query', root, {
-              'expression' => ['type', 'f'],
+            query = ['query', watch_root, {
+              'expression' => relative_path ? ['allof', ['type', 'f'], ['dirname', relative_path]] : ['type', 'f'],
               'fields'     => ['name'],
             }]
             paths = Watchman::Utils.query(query, socket)
 
             # could return error if watch is removed
             extracted = extract_value(paths, 'files')
+            extracted.each { |p| p.delete_prefix!("#{relative_path}/") } if relative_path
             if @wildignore
               extracted.select { |path| path !~ @wildignore }
             else

8000
@wincent
Copy link
Owner
wincent commented Jun 17, 2022

Do we have to worry about supporting old versions of watchman that don't support watch-project?

We probably should, so as not to break people's set-ups... One hacky way to do it that springs to mind is to scan the output of watchman --help to see if watch-project is a substring. Even though it's a hack, it's probably better (less annoying to users) than providing a configuration option.

The other question that comes to mind is whether there is any performance impact from the delete_prefix! calls... if it's negligible, great; if it's not, then continuing to use the deprecated command may be desirable for some users (ie. those who want to trade memory for speed).

@mr-salty
Copy link
Author

yeah, I was worried about all the extra string data, plus the cost of stripping it, but I found a better option that takes care of that inside watchman - relative_root: https://facebook.github.io/watchman/docs/file-query.html#relative-roots . I updated my script and confirmed the paths you get back are relative to relative_root so no prefix stripping is needed.

one issue is that says it was introduced in 3.3 whereas watch-project was 3.1 so checking for watch-project won't work but maybe we can check watchman --version. FWIW, the oldest release even shown on https://facebook.github.io/watchman/docs/release-notes.html is 3.3.0 and that's from 2015-06-22 (so i'll have to try to find an old version and confirm --version even exists).

mr-salty pushed a commit to mr-salty/command-t that referenced this issue Jun 17, 2022
feat: use watchman's watch-project if available
d6623cf 
If watchman is bersion 3.3 or later, use `watch-project` rather than
the deprecated `watch`. The main advantage is eliminating redundant
nested watches (i.e.  where one is a subtree of another), which reduces
RAM and CPU usage in the `watchman` daemon and eliminates the indexing
delay when invoking Command-T after moving down the directory hierarchy.

Although `watch-project` was introduced in 3.1 it is awkward/inefficient
to use without the `relative_root` query which was introduced in 3.3.
3.3 was released in June 2015, so most users should have a newer version
but backward compatibility with older versions is maintained.

Fixes wincent#389

Tested:
* Tested the new code under various scenarios by invoking Command-T in my homedir
  and various subdirectories, as well as other paths (/tmp).
* Manually tested the fallback path to `watch`, which is unchanged from existing code.
* Tested various version numbers by manually simulating the output of `--version`
    * If the version number is missing or malformed, it will end up as version 0.0
    * test cases: nil, "", "foo.bar", "-5.2", "1", "3.2", 3.3.449", "4", "2022.06.13.00"
@mr-salty mr-salty mentioned this issue Jun 17, 2022
Closed
Padmamanickam added a commit to Padmamanickam/command-t that referenced this issue Feb 6, 2023
@Padmamanickam @wincent
perf: use watchman's watch-project if available
c55210b 
If watchman is version 3.3 or later, use `watch-project` rather than
the deprecated `watch`. The main advantage is eliminating redundant
nested watches (i.e.  where one is a subtree of another), which reduces
RAM and CPU usage in the `watchman` daemon and eliminates the indexing
delay when invoking Command-T after moving down the directory hierarchy.

Although `watch-project` was introduced in 3.1 it is awkward/inefficient
to use without the `relative_root` query which was introduced in 3.3.
3.3 was released in June 2015, so most users should have a newer version
but backward compatibility with older versions is maintained.

Fixes: wincent/command-t#389

Tested:
* Tested the new code under various scenarios by invoking Command-T in
  my homedir and various subdirectories, as well as other paths (/tmp).
* Manually tested the fallback path to `watch`, which is unchanged from
  existing code.
* Tested various version numbers by manually simulating the output of
  `--version`
    * If the version number is missing or malformed, it will end up as
      version 0.0
    * test cases: nil, "", "foo.bar", "-5.2", "1", "3.2", 3.3.449", "4",
      "2022.06.13.00"

See also:

- https://facebook.github.io/watchman/docs/cmd/watch.html
- https://facebook.github.io/watchman/docs/cmd/watch-project.html

Signed-off-by: Greg Hurrell <greg@hurrell.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: use watchman's watch-project if available mr-salty/command-t
2 participants
@wincent @mr-salty
0