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

Make independent owners requests per file to improve memoization #10491

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Jul 28, 2020

Problem

#10441 caused a performance regression from about 10s to run ./pants dependencies --transitive :: with inference enabled, to about 22s.

Solution

Make independent owners requests per file we'd like to find owners, which allows the lookups for each file to be memoized independently.

Result

./pants dependencies --transitive :: takes 15s. Although we are doing more work than before (before #10441, conftest discovery would only happen at test time), this is not ideal, and we should do further optimization before launch. But there are a few variables that will impact this soon that make it not the best time to optimize: 1) an intrinisic PathGlobs->Paths operation, 2) possibly enabling inference by default, allowing all of the strategies to use the module_mapper.

[ci skip-rust-tests]

# Rust tests will be skipped. Delete if not intended.
[ci skip-rust-tests]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. How would you feel about instead using the map already created for import inference? If you do that, what's the result time?

I suspect we'll want to use that map so that we avoid any unnecessary work, and so that there isn't a performance consideration on whether you want import inference or not. The decision to use import inference would only be if you like the behavior or not.

@stuhood
Copy link
Sponsor Member Author

stuhood commented Jul 28, 2020

@Eric-Arellano : I mentioned it in the Result section: it felt like it would be a good idea to tie using that map to inference being enabled by default. Because with no import inference, __init__.py and conftest.py will still be lazily discovered, unlike with import inference, which needs to scan the world. Once that is enabled by default it will be an easier decision, and one optimization path.

@stuhood stuhood merged commit 8c95a5b into pantsbuild:master Jul 28, 2020
@stuhood stuhood deleted the stuhood/init-conftest-inference-optimization branch July 28, 2020 17:23
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

Successfully merging this pull request may close these issues.

None yet

2 participants