FindModuleCache: leverage BuildSourceSet #9478

huguesb · 2020-09-24T19:50:43Z

Given a large codebase with folder hierarchy of the form

foo/
    company/
        __init__.py
        foo/
bar/
    company/
        __init__.py
        bar/
baz/
    company/
        __init__.py
        baz/
    ...

with >100 toplevel folders, the time spent in load_graph
is dominated by find_module because this operation is
itself O(n) where n is the number of input files, which
ends up being O(n**2) because it is called for every import
statement in the codebase and the way find_module work,
it will always scan through each and every one of those
toplevel directories for each and every import statement
of company.*

Introduce a fast path that leverages the fact that for
imports within the code being typechecked, we already
have a mapping of module import path to file path in
BuildSourceSet

In a real-world codebase with ~13k files split across
hundreds of packages, this brings load_graph from
~180s down to ~48s, with profiling showing that parse
is now taking the vast majority of the time, as expected.

huguesb · 2020-09-24T19:53:37Z

mypy/modulefinder.py

+                # another 'bar' module, because it's a waste of time and even in the
+                # unlikely event that we did find one that matched, it probably would
+                # be completely unrelated and undesirable
+                return ModuleNotFoundReason.NOT_FOUND


this is probably the most controversial behavior change, but unfortunately,also the key ingredient in achieving any measurable speedup, because most calls to find_module will fail, e.g. from typing import Dict triggers a call to find_module('typing.Dict')...

gvanrossum

Interesting. I still have to get comfortable with this, but I tried to understand what you're doing and review the code and comments for style and clarity.

I wonder if there's a different way to speed things up, e.g. change fscache.py to use presence or absence in the listdir cache for a directory to decide whether a file exists, rather than call stat() (maybe only call stat() if it does exist).

Also, this is pretty subtle code, and I think you discovered this while you were working on this! Even though this probably gets exercised quite a bit by other unit tests, maybe you should add some tests that specifically try to at least test that all the code paths (both successes and failures) work? (If a bug were to be introduced here, it would be pretty nasty to track it down based on failures in other tests.)

gvanrossum · 2020-09-27T00:07:37Z

mypy/modulefinder.py

+        # fast path for any modules in the current source set
+        # this is particularly important when there are a large number of search
+        # paths which share the first (few) component(s) due to the use of namespace
+        # packages, for instance


Punctuation.

Suggested change

# fast path for any modules in the current source set

# this is particularly important when there are a large number of search

# paths which share the first (few) component(s) due to the use of namespace

# packages, for instance

# Fast path for any modules in the current source set.

# This is particularly important when there are a large number of search

# paths which share the first (few) component(s) due to the use of namespace

# packages, for instance:

#

(The other paragraphs below are also missing periods.)

gvanrossum · 2020-09-27T00:08:34Z

mypy/modulefinder.py

@@ -92,6 +93,33 @@ def __repr__(self) -> str:
            self.base_dir)


+class BuildSourceSet:
+    """Efficiently test a file's membership in the set of build sources."""


This docstring has always bothered me.

Suggested change

"""Efficiently test a file's membership in the set of build sources."""

"""Helper to efficiently test membership in a set of build sources."""

mypy/modulefinder.py

gvanrossum · 2020-09-27T00:14:01Z

mypy/modulefinder.py

+        #        __init__.py
+        #        baz/
+        #
+        # mypy gets [foo/company/foo, foo/company/bar, foo/company/baz, ...] as input


Also is this supposed to say bar/company/par, baz/company/baz?

Yes, good catch.

gvanrossum · 2020-09-27T00:19:50Z

mypy/modulefinder.py

+                #   2. foo.bar.py[i] is in the source set
+                #   3. foo.bar is not a directory


Got your slashes and dots mixed up?

Suggested change

# 2. foo.bar.py[i] is in the source set

# 3. foo.bar is not a directory

# 2. foo/bar.py[i] is in the source set

# 3. foo/bar is not a directory

Or perhaps "3. foo.bar is not a package"?

Also, I find it hard to match these three conditions with the three tests actually being made. Point (1) is not really a condition, it just sets the context: id == 'foo.bar.baz', id[:idx] == 'foo.bar', and parent is either 'foo/bar.py' or foo/bar/__init__.py[i]). Point (2) is the check for __init__.py. Point (3) is the isdir() check. Together points (2) and (3) verify that foo.bar is a module but not a package. Have I got that right?

Maybe you can work the example from your GitHub comment into the source comment here to clarify things?

gvanrossum · 2020-09-27T00:26:29Z

mypy/modulefinder.py

+            # otherwise we might have false positives compared to slow path
+            d = os.path.dirname(p)
+            for i in range(id.count('.')):
+                if not self.fscache.isfile(os.path.join(d, '__init__.py')):


Shouldn't this check for __init__.pyi too? That makes a package too.

Good catch!

gvanrossum · 2020-09-27T00:30:32Z

mypy/modulefinder.py

+            return p
+
+        idx = id.rfind('.')
+        if idx != - 1:


PEP8

Suggested change

if idx != - 1:

if idx != -1:

gvanrossum · 2020-09-27T00:36:58Z

mypy/modulefinder.py

+            parent = self.find_module_via_source_set(id[:idx])
+            if (
+                    parent and isinstance(parent, str)
+                    and not parent.endswith('__init__.py')


This feels dicey -- what if there's a module named foo__init__.py? Also, (again) why not check for __init__.pyi?

Hmm, yes, I suppose this check should include a path separator. And, yes, checking for pyi as well

gvanrossum · 2020-09-27T00:40:05Z

mypy/modulefinder.py

+                    parent and isinstance(parent, str)
+                    and not parent.endswith('__init__.py')
+                    and not self.fscache.isdir(os.path.splitext(parent)[0])
+            ):


We don't really use this black-like formatting in mypy.

gvanrossum · 2020-09-27T00:45:09Z

mypy/modulefinder.py

+        if idx != - 1:
+            parent = self.find_module_via_source_set(id[:idx])
+            if (
+                    parent and isinstance(parent, str)


Do you need the parent and? I think that if parent is None it's not an instance of str, and if it's a string, it won't be empty. If you do need it, I'd move it to a separate line (to emphasize that there really are four conditions being checked) and if you can change it to parent is not None.

huguesb · 2020-10-18T08:43:04Z

I wonder if there's a different way to speed things up,

I've considered a couple other approaches but this seemed to me the most elegant and efficient. I'm open to other ideas, but, as I mentioned in my first comment, the key ingredient to the speedup is cutting short the search when we've found a valid parent module, and I don't think filesystem cache can really address that.

Also, this is pretty subtle code, and I think you discovered this while you were working on this! Even though this probably gets exercised quite a bit by other unit tests, maybe you should add some tests that specifically try to at least test that all the code paths (both successes and failures) work? (If a bug were to be introduced here, it would be pretty nasty to track it down based on failures in other tests.)

It is indeed pretty subtle. It is covered quite extensively by other tests, but you're right that tracking bugs via such indirect coverage is tricky. What general form do you think would be best for tests to most directly cover this area?

huguesb · 2022-01-21T07:47:17Z

Rebasing this on top of 0.931

The impact is even larger now. With vanilla 0.931, our codebase takes 29min to typecheck, with this patch, the time drops under 8min (both mypyc-enabled build).

I believe I have addressed all of @gvanrossum 's comments. I have also taken the extra step to gate the fast path behind a command line switch to assuage concerns about it causing subtle breakages in the wild.

Gated behind a command line flag to assuage concerns about subtle issues in module lookup being introduced by this fast path.

gvanrossum · 2022-02-20T21:23:41Z

I'm sorry I haven't had time to look into this. I still have some hope that I'll find the time before the next mypy release.

huguesb · 2022-04-18T06:49:24Z

Apologies for the accidental branch deletion. Re-opened as #12616 (rebased on master)

huguesb commented Sep 24, 2020

View reviewed changes

huguesb force-pushed the pr-module-finder branch from 04bc7f6 to ff60027 Compare September 24, 2020 21:41

gvanrossum reviewed Sep 27, 2020

View reviewed changes

huguesb force-pushed the pr-module-finder branch from ff60027 to 4cfdab1 Compare January 21, 2022 07:43

FindModuleCache: optionally leverage BuildSourceSet

4ab2035

Gated behind a command line flag to assuage concerns about subtle issues in module lookup being introduced by this fast path.

huguesb force-pushed the pr-module-finder branch from 4cfdab1 to 4ab2035 Compare January 21, 2022 07:51

97littleleaf11 requested a review from JukkaL January 21, 2022 11:16

97littleleaf11 added this to Waiting for review in mypy Jan 21, 2022

97littleleaf11 added the performance label Jan 21, 2022

97littleleaf11 mentioned this pull request Apr 14, 2022

Release 0.950 planning #12579

Closed

huguesb closed this Apr 18, 2022

huguesb deleted the pr-module-finder branch April 18, 2022 05:47

mypy automation moved this from Waiting for review to Done Apr 18, 2022

huguesb mentioned this pull request Apr 18, 2022

FindModuleCache: optionally leverage BuildSourceSet #12616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FindModuleCache: leverage BuildSourceSet #9478

FindModuleCache: leverage BuildSourceSet #9478

huguesb commented Sep 24, 2020 •

edited

huguesb Sep 24, 2020

gvanrossum left a comment

gvanrossum Sep 27, 2020

gvanrossum Sep 27, 2020

gvanrossum Sep 27, 2020

huguesb Oct 18, 2020

gvanrossum Sep 27, 2020

gvanrossum Sep 27, 2020

huguesb Oct 18, 2020

gvanrossum Sep 27, 2020

gvanrossum Sep 27, 2020

huguesb Oct 18, 2020

gvanrossum Sep 27, 2020

gvanrossum Sep 27, 2020

huguesb commented Oct 18, 2020 •

edited

huguesb commented Jan 21, 2022 •

edited

gvanrossum commented Feb 20, 2022

huguesb commented Apr 18, 2022

	"""Efficiently test a file's membership in the set of build sources."""
	"""Helper to efficiently test membership in a set of build sources."""

		# 2. foo.bar.py[i] is in the source set
		# 3. foo.bar is not a directory

FindModuleCache: leverage BuildSourceSet #9478

FindModuleCache: leverage BuildSourceSet #9478

Conversation

huguesb commented Sep 24, 2020 • edited

Choose a reason for hiding this comment

gvanrossum 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

huguesb commented Oct 18, 2020 • edited

huguesb commented Jan 21, 2022 • edited

gvanrossum commented Feb 20, 2022

huguesb commented Apr 18, 2022

huguesb commented Sep 24, 2020 •

edited

huguesb commented Oct 18, 2020 •

edited

huguesb commented Jan 21, 2022 •

edited