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

Remove usage of @memoized_property on MappedSpecs #6551

Merged
merged 1 commit into from Sep 25, 2018

Conversation

Projects
None yet
4 participants
@stuhood
Copy link
Member

stuhood commented Sep 24, 2018

Problem

Due to some unexpected behaviour of @memoized_property, #6480 made spec parsing accidentally quadratic. See #6550 for a general overview of the problem.

Solution

Because @memoized_property was still a good strategy for the purposes of compiled regexes and compiled tag matchers, this extracts the inputs to those properties to a datatype SpecsMatcher, which will have a much smaller instance. It then removes _MappedSpecs by inlining its two remaining methods into addresses_from_specs to avoid other unnecessary memoization.

Result

For:

time ./pants --changed-diffspec='59d1632aa7bd4d7713d5ac18ae401adc7631e070^..59d1632aa7bd4d7713d5ac18ae401adc7631e070' --changed-include-dependees=transitive list | wc -l
# before #6480
real	0m7.574s
user	0m8.323s
sys	0m2.848s
 
# after #6480
real	0m17.996s
user	0m18.529s
sys	0m3.000s
 
# after this patch
real	0m7.603s
user	0m8.233s
sys	0m2.786s

#6550 discusses auditing other usages of @memoized_property.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Sep 25, 2018

As mentioned in #6550 I think this could be obviated by caching the __hash__ of datatypes, but the separation introduced here is good anyway, even if it's for perf reasons, I think.


def _matching_addresses_for_spec(self, spec):
matched_addresses = OrderedSet()

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Sep 25, 2018

Contributor

If this has specific relevance to the perf numbers described in the OP compared to the memoized property part, would leave a comment here in addition to this change.

@stuhood stuhood merged commit f60c40a into pantsbuild:master Sep 25, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/specs-no-memoized-property-on-large-instances branch Sep 25, 2018

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