Overhaul file-system operations to use FileSystemCache #4623

msullivan · 2018-02-23T01:07:36Z

This overhauls our file-system accesses to always go through a FileSystemCache when stat()ing or reading source files. (We don't use it for reading cache json files, since we seem to only do a single read on each anyways and so caching doesn't help us any.) This eliminates a number of potential bugs having to do with seeing inconsistent file-system state and reduces the number of file reads+decodes in fine-grained incremental mode.

This also allows us to eliminate a number of caches maintained by find_module that duplicated functionality of FileSystemCache. Since I was reluctant to make the FileSystemCache global, I took the opportunity to make find_module's caches non-global as well.

msullivan · 2018-02-23T07:48:39Z

Argh: on windows python generates different error messages in the FileNotFoundError for open and os.stat, and this changes things to do os.stat first.

They'll still merge conflict, but the resolution will be trivial now (I'm trying avoid making one depend on the other)

JukkaL

Excellent, this should improve performance and this will make it easier to reason about correctness when there are concurrent file system updates. Also thanks for refactoring things a little while you were are it!

I have just a few minor things (also you need to fix a merge conflict). Finally, before merging, I'd like a few more steps:

Measure the performance impact to a trivial fine-grained incremental update (preferably on macOS, since FS operations tend to be slow on macs).
Manually verify that file system changes are still correctly picked up in some common scenarios (both with daemon and without).

JukkaL · 2018-03-02T13:28:22Z

mypy/server/update.py

@@ -499,13 +505,14 @@ def get_module_to_path_map(manager: BuildManager) -> Dict[str, str]:
            for module, node in manager.modules.items()}


-def get_sources(modules: Dict[str, str],
+def get_sources(fscache: FileSystemCache,
+                modules: Dict[str, str],
                changed_modules: List[Tuple[str, str]]) -> List[BuildSource]:
    # TODO: Race condition when reading from the file system; we should only read each


Can this TODO comment be removed now?

JukkaL · 2018-03-02T13:33:35Z

mypy/build.py

@@ -544,6 +548,23 @@ def find_config_file_line_number(path: str, section: str, setting_name: str) ->
    return -1


+class FindModuleCache:


Add docstring. Mention that this never flushes the FileSystemCache and caller is responsible for that.

JukkaL · 2018-03-02T13:35:21Z

mypy/build.py

-
-
-def find_module(id: str, lib_path_arg: Iterable[str]) -> Optional[str]:
+def find_module(cache: FindModuleCache, id: str, lib_path_arg: Iterable[str]) -> Optional[str]:


Should this be a method of FindModuleCache? If not, document in FindModuleCache that this function populates the data structures and is tightly coupled with the class.

JukkaL · 2018-03-02T13:36:27Z

mypy/build.py

            dirs = []
            for pathitem in lib_path:
                # e.g., '/usr/lib/python3.4/foo/bar'
-                isdir = find_module_isdir_cache.get((pathitem, dir_chain))


Why was this block of code removed?

It's maintaining an isdir cache, which is handled by the fscache stat cache.

msullivan · 2018-03-10T00:45:29Z

OK, all of the other changes I was making that conflicted with this are in now, so I've updated it all and made the requested cleanups.

Doesn't affect the performance of a trivial fine-grained incremental update at all.

Manual testing seems to pick up changes fine.

gvanrossum · 2018-03-11T02:14:33Z

@ethanhs Note that this will most likely affect your PEP 561 PR (#4693) severely.

emmatyping · 2018-03-11T06:09:28Z

@gvanrossum, indeed. I was originally hoping to get my old PR merged before this one was opened, but that of course didn't happen. The good news is my implementation in #4693 was in a single commit, so I should be able to revert that, rebase on master and re-apply the changes (with slight modification) without too much headache. Thanks for the heads up!

PS: I originally interpreted your comment to mean that this is getting merged before my PR, is that the case?

JukkaL

There are a few additional remaining non-cached os operations in mypy.build (also see comments):

getmtime (os.path.getmtime)
remove_cwd_prefix_from_path (os.path.isfile)

Should these be updated to use the cache as well?

JukkaL · 2018-03-12T15:22:55Z

mypy/build.py


-def list_dir(path: str) -> Optional[List[str]]:
-    """Return a cached directory listing.
+       Module locations and some intermediate results are cached internally


Style nit: Indents seem off unless github diff view is acting up. Just to be extra clear, indents should be like this:

class Foo: """First line. Second line (4 space indent). """

JukkaL · 2018-03-12T15:23:54Z

mypy/build.py

+            # use hits to avoid adding it a second time when we see x.pyi.
+            # This also avoids both x.py and x.pyi when x/ was seen first.
+            hits = set()  # type: Set[str]
+            for item in sorted(os.listdir(os.path.dirname(module_path))):


Should we use FileSystemCache here and below?

gvanrossum · 2018-03-11T02:26:08Z

mypy/dmypy_server.py

-        manager = result.manager
-        graph = result.graph
-        self.fine_grained_manager = FineGrainedBuildManager(manager, graph)
+        self.fine_grained_manager = mypy.server.update.FineGrainedBuildManager(result)


IIUC the imports still have from mypy.server.update import FineGrainedBuildManager so why the module prefix?

Improperly resolved a merge conflict. When this patch first went up it used a prefix, but I changed it to not in another patch and must have undone it while merging.

gvanrossum · 2018-03-12T16:52:31Z

mypy/main.py

@@ -527,7 +527,7 @@ def add_invertible_flag(flag: str,
                 .format(special_opts.package))
        options.build_type = BuildType.MODULE
        lib_path = [os.getcwd()] + build.mypy_path()
-        targets = build.find_modules_recursive(special_opts.package, lib_path)
+        targets = build.FindModuleCache().find_modules_recursive(special_opts.package, lib_path)


It's kind of sad that this creates a separate instance of all the caches, and introduces an opportunity for a race condition. Maybe add a TODO (since it was a pre-existing condition)?

gvanrossum · 2018-03-12T16:54:50Z

mypy/fscache.py



 class FileSystemCache:
-    def __init__(self, pyversion: Tuple[int, int]) -> None:
+    def __init__(self, pyversion: Optional[Tuple[int, int]] = None) -> None:


I wonder if there shouldn't be two classes here. A base class that has no python_version and no read_with_python_encoding() method, and a subclass that has both. This would avoid the Optional and the assert (I had to go on a hunt for all instantiations of this class to get comfortable with your current version, and it seems all but one call site would be happy with the proposed base class.)

gvanrossum · 2018-03-12T16:57:18Z

mypy/build.py

+        lib_path = tuple(lib_path_arg)
+        fscache = self.fscache
+
+        def find() -> Optional[str]:


While we're refactoring, I wonder if this should be made a regular method (with a longer name) and passed in whatever it needs from find_module(). The nested function always bothered me, and a nested function that references self bothers me more.

gvanrossum · 2018-03-12T18:34:25Z

mypy/build.py

-
-
-def verify_module(id: str, path: str) -> bool:
+def verify_module(fscache: FileSystemCache, id: str, path: str) -> bool:


Maybe this could become a method on FileSystemCache?

I'd rather not add more knowledge about python-specific things to FileSystemCache

gvanrossum · 2018-03-12T18:35:55Z

mypy/build.py

@@ -1399,14 +1325,6 @@ def delete_cache(id: str, path: str, manager: BuildManager) -> None:
 d. from P import M; checks filesystem whether module P.M exists in
   filesystem.

-e. Race conditions, where somebody modifies a file while we're


Maybe keep the bullet with the first sentence and replace the rest with "Solved by FileSystemClass"?

gvanrossum · 2018-03-12T22:08:41Z

mypy/build.py

                except IOError as ioerr:
+                    # ioerr.strerror differs for os.stat failures between Windows and
+                    # other systems, but os.strerror(ioerr.errno) does not, so we use that.


Maybe explain that we want the error to be platform-independent so tests can have predictable output?

gvanrossum · 2018-03-12T22:45:15Z

mypy/fscache.py

-        return stat.S_ISREG(st.st_mode)
+        try:
+            st = self.stat(path)
+            return stat.S_ISREG(st.st_mode)


Possibly move this out of the try/except -- this can't raise OSError.

gvanrossum · 2018-03-12T22:47:27Z

mypy/fscache.py

+
+        On case-insensitive filesystems (like Mac or Windows) this returns
+        False if the case of the path's last component does not exactly
+        match the case found in the filesystem.


Hm, this means that importing wrongly-cased packages will not be flagged, since we only compare the filename (which would be __init__.py for a package). Not actionable except perhaps add a TODO comment; this was a pre-existing problem. (Python itself gets it right so that's a fallback.)

msullivan · 2018-03-13T23:42:35Z

I updated remove_cwd_prefix_from_path to use the cache, but getmtime is only used for cache json files, which we don't use the fscache for.

gvanrossum · 2018-03-14T02:13:59Z

Yay!

msullivan requested review from JukkaL, gvanrossum and ilevkivskyi February 23, 2018 01:07

msullivan force-pushed the load_graph_opt branch from 4da63be to 7520f89 Compare February 28, 2018 05:34

carljm mentioned this pull request Feb 28, 2018

[WIP] Namespace implementation #4277

Closed

msullivan force-pushed the use_fscache branch from bc63840 to 72820c0 Compare March 1, 2018 00:43

msullivan added 8 commits March 1, 2018 11:01

Make BuildManager use an fscache

a324d38

Some cleanups

6aa62e5

Make update.py use fscache

a8d8134

Move read_with_python_encoding

c9d1d58

Huge rampage through get_module stuff

dcb7aa9

Remove some dead code

788c8c5

Fix an error message mismatch on windows.

11094ee

Fix stubgen after move

afe715d

msullivan force-pushed the use_fscache branch from 72820c0 to ebdf7d1 Compare March 1, 2018 19:02

msullivan changed the base branch from load_graph_opt to master March 1, 2018 19:03

Remove some now out-of-date comments.

893e1da

msullivan force-pushed the use_fscache branch from ebdf7d1 to 893e1da Compare March 1, 2018 19:06

Tweak flushing logic to better match #4664

0f5a411

They'll still merge conflict, but the resolution will be trivial now (I'm trying avoid making one depend on the other)

msullivan force-pushed the use_fscache branch from 4c6d59e to 0f5a411 Compare March 2, 2018 04:21

JukkaL reviewed Mar 2, 2018

View reviewed changes

carljm mentioned this pull request Mar 5, 2018

Implement PEP 561 searching #4403

Closed

8 tasks

msullivan mentioned this pull request Mar 7, 2018

Make testfinegrained use dmypy_server #4699

Merged

msullivan added 5 commits March 7, 2018 14:34

SQUASHED MERGE COMMIT WITH dmypy-fg-testing

f516473

Merge branch 'master' into use_fscache

2691e1f

SQUASHED MERGE COMMIT WITH delete_optimize

f71de12

Merge branch 'master' into use_fscache2

9afa0da

Refactor find_module into the cache class, doc tweaks

5a15f68

emmatyping mentioned this pull request Mar 11, 2018

Implement PEP561 #4693

Merged

9 tasks

JukkaL reviewed Mar 12, 2018

View reviewed changes

gvanrossum reviewed Mar 12, 2018

View reviewed changes

Merge branch 'master' into use_fscache

c5c2492

msullivan added 3 commits March 13, 2018 16:57

Some cleanups

7869dd6

Refactor a bit

7fb7055

Refactor fscache into two classes

ea76cc7

msullivan merged commit 5ac30ea into master Mar 14, 2018

msullivan deleted the use_fscache branch March 14, 2018 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul file-system operations to use FileSystemCache #4623

Overhaul file-system operations to use FileSystemCache #4623

msullivan commented Feb 23, 2018 •

edited

Loading

msullivan commented Feb 23, 2018

JukkaL left a comment

JukkaL Mar 2, 2018

JukkaL Mar 2, 2018

JukkaL Mar 2, 2018

JukkaL Mar 2, 2018

msullivan Mar 2, 2018

msullivan commented Mar 10, 2018

gvanrossum commented Mar 11, 2018

emmatyping commented Mar 11, 2018 •

edited

Loading

JukkaL left a comment

JukkaL Mar 12, 2018

JukkaL Mar 12, 2018

gvanrossum Mar 11, 2018

msullivan Mar 13, 2018

gvanrossum Mar 12, 2018

gvanrossum Mar 12, 2018

gvanrossum Mar 12, 2018

gvanrossum Mar 12, 2018

msullivan Mar 13, 2018

gvanrossum Mar 14, 2018

gvanrossum Mar 12, 2018

gvanrossum Mar 12, 2018

gvanrossum Mar 12, 2018

gvanrossum Mar 12, 2018

msullivan commented Mar 13, 2018

gvanrossum commented Mar 14, 2018

		@@ -544,6 +548,23 @@ def find_config_file_line_number(path: str, section: str, setting_name: str) ->
		return -1


		class FindModuleCache:



		def find_module(id: str, lib_path_arg: Iterable[str]) -> Optional[str]:
		def find_module(cache: FindModuleCache, id: str, lib_path_arg: Iterable[str]) -> Optional[str]:



		def verify_module(id: str, path: str) -> bool:
		def verify_module(fscache: FileSystemCache, id: str, path: str) -> bool:

Overhaul file-system operations to use FileSystemCache #4623

Overhaul file-system operations to use FileSystemCache #4623

Conversation

msullivan commented Feb 23, 2018 • edited Loading

msullivan commented Feb 23, 2018

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msullivan commented Mar 10, 2018

gvanrossum commented Mar 11, 2018

emmatyping commented Mar 11, 2018 • edited Loading

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msullivan commented Mar 13, 2018

gvanrossum commented Mar 14, 2018

msullivan commented Feb 23, 2018 •

edited

Loading

emmatyping commented Mar 11, 2018 •

edited

Loading