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

Fix incremental speed regression #4331

Merged
merged 5 commits into from Dec 8, 2017

Conversation

Projects
None yet
2 participants
@gvanrossum
Member

gvanrossum commented Dec 6, 2017

Fixes #4135.

@gvanrossum gvanrossum requested a review from JukkaL Dec 6, 2017

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Dec 7, 2017

Member

Don't merge yet -- this has an O(N**2) component. We can do better by having an "errors" bit per file. (Though it's really per SCC that counts.)

Member

gvanrossum commented Dec 7, 2017

Don't merge yet -- this has an O(N**2) component. We can do better by having an "errors" bit per file. (Though it's really per SCC that counts.)

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Dec 8, 2017

Member

What do you think of this version? It sets a flag transitive_error when there's an error in either the current module or any of its dependencies. (Except it does it it per SCC, so if any dependency of any module in an SCC has this flag set, all modules in the SCC get it set.)

There's one bit of strangeness (see the last local commit) -- one test decided to have a different [stale] list.

(If you'd rather see the previous O(N**2) version, I can probably get it back using git reflog. :-)

Member

gvanrossum commented Dec 8, 2017

What do you think of this version? It sets a flag transitive_error when there's an error in either the current module or any of its dependencies. (Except it does it it per SCC, so if any dependency of any module in an SCC has this flag set, all modules in the SCC get it set.)

There's one bit of strangeness (see the last local commit) -- one test decided to have a different [stale] list.

(If you'd rather see the previous O(N**2) version, I can probably get it back using git reflog. :-)

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Dec 8, 2017

Collaborator

The approach looks good to me. I'm a little worried about the difference in the test case though -- might be worth figuring out why it has changed. Feel free to merge this once you are happy with this (and have fixed Travis).

Collaborator

JukkaL commented Dec 8, 2017

The approach looks good to me. I'm a little worried about the difference in the test case though -- might be worth figuring out why it has changed. Feel free to merge this once you are happy with this (and have fixed Travis).

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Dec 8, 2017

Member

Ah, I understand why that test (testIncrementalWithSilentImportsAndIgnore) had to be modified. There are no errors in c/__init__.py, and it does not depend on modules with errors. However it is processed after a module with errors (c/submodule.py), and with the old approach that meant no cache file would be written for c/__init__.py (in fact that was the bug we're fixing here). But with the new approach the file is not considered to have a (transitive) error, and the logic in write_cache() decides it can write the cache file. So it's a good thing.

Member

gvanrossum commented Dec 8, 2017

Ah, I understand why that test (testIncrementalWithSilentImportsAndIgnore) had to be modified. There are no errors in c/__init__.py, and it does not depend on modules with errors. However it is processed after a module with errors (c/submodule.py), and with the old approach that meant no cache file would be written for c/__init__.py (in fact that was the bug we're fixing here). But with the new approach the file is not considered to have a (transitive) error, and the logic in write_cache() decides it can write the cache file. So it's a good thing.

@gvanrossum gvanrossum merged commit 01de89f into python:master Dec 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gvanrossum gvanrossum deleted the gvanrossum:fix-4135-incremental-speed branch Dec 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment