bpo-31324: Optimize support._match_test() by vstinner · Pull Request #4421 · python/cpython

vstinner · 2017-11-16T15:16:44Z

Remove support.match_tests global variable. It is replaced with a
new support.set_match_tests() function, so _match_test() doesn't
have to check each time if patterns were modified.
Rewrite _match_test() with a new _MatchTests object which uses
different code paths depending on the kind of patterns.

Co-Authored-By: Serhiy Storchaka [email protected]

https://bugs.python.org/issue31324

vstinner · 2017-11-16T15:22:11Z

Lib/test/support/__init__.py:

-match_tests = None

Only Lib/test/libregrtest/ was using support.match_tests, and my PR updates Lib/test/libregrtest/. I don't think that support.mtach_tests is used outside CPython code base. test.support is mostly used internally by the CPython test suite.

serhiy-storchaka · 2017-11-16T15:25:37Z

Not needed.

Fixed ("global" removed)

serhiy-storchaka · 2017-11-16T15:27:09Z

I think the class is not needed. Just use a function which returns a function.

serhiy-storchaka · 2017-11-16T15:43:02Z

Just return set(patterns).__contains__.

serhiy-storchaka · 2017-11-16T15:46:11Z

What about '?' and '['?

serhiy-storchaka · 2017-11-16T15:47:23Z

Could be all(map(self._is_full_match, patterns)).

* Rename support._match_test() to support.match_test(): make it public * Remove support.match_tests global variable. It is replaced with a new support.set_match_tests() function, so match_test() doesn't have to check each time if patterns were modified. * Rewrite match_test(): use different code paths depending on the kind of patterns for best performances. Co-Authored-By: Serhiy Storchaka <[email protected]>

vstinner · 2017-11-21T13:18:16Z

@serhiy-storchaka: I addressed your comments, I added unit tests, I chose to make support.match_test() public since it's used by regrtest, and I rebased my change.

serhiy-storchaka

Added few suggestions.

And please add tests with a pattern containing '.' and metacharacters and with multiple patterns.

serhiy-storchaka · 2017-11-21T13:32:45Z

+    #
+    # Reject patterns which contain fnmatch patterns: '*', '?', '[...]'
+    # or '[!...]'. For example, reject 'test_access*'.
+    return ('.' in pattern) and all(char not in '?*[]' for char in pattern)


I'm wondering whether using a regular expression would be faster.

return ('.' in pattern) and not re.search(r'[?*\[\]]', pattern)

I think that (char not in '?*[]') is more readable, and this function doesn't seem to be a performance bottleneck, there is no need to optimize it. Moreover, I just added a cache to not recompile patterns if set_match_tests() is called twice with the same patterns.

It seems like the compilation to a giant regex is slow, but I don't think that we can optimize this much, and again, I don't think that it's worth it. My main usage of many patterns is when I use test.bisect, but test.bisect only uses full test identifiers and so should use set().contains.

serhiy-storchaka · 2017-11-21T13:37:01Z

+
+    if not patterns:
+        _match_test_func = None
+    elif all(_is_full_match_test(pattern) for pattern in patterns):


Could be all(map(_is_full_match_test, patterns)).

Right, but I prefer list comprehensions. IMHO it's more readable.

map() is already used twice few lines below.

Oh right, that's the copied I copied from your PR :-D You won, I updated my PR to address your latest review.

serhiy-storchaka · 2017-11-21T13:46:47Z

+        test_access = Test('test.test_os.FileTests.test_access')
+        test_chdir = Test('test.test_os.Win32ErrorTests.test_chdir')
+
+        old_match = support._match_test_func


Could use

with support.swap_attr(support, '_match_test_func', None):

Use also support.swap_attr().

vstinner · 2017-11-21T14:13:13Z

And please add tests with a pattern containing '.' and metacharacters and with multiple patterns.

Done

serhiy-storchaka · 2017-11-21T16:10:10Z

    # Reject patterns which contain fnmatch patterns: '*', '?', '[...]'
    # or '[!...]'. For example, reject 'test_access*'.
-    return ('.' in pattern) and all(char not in '?*[]' for char in pattern)
+    return ('.' in pattern) and (not re.search(r'[?*\[\]]', pattern))


Are parenthesis around not ... add readability?

serhiy-storchaka · 2017-11-21T16:15:41Z

+
+        func = match_test_regex
+
+    _match_test_patterns = patterns


Shouldn't we make a copy? Or just ignore the case of modifying patterns after calling set_match_tests()? This is a private API at end.

Right, fixed.

vstinner · 2017-11-21T23:34:22Z

Thank you for the reviews @serhiy-storchaka!

* bpo-31324: Optimize support._match_test() (#4421) * Rename support._match_test() to support.match_test(): make it public * Remove support.match_tests global variable. It is replaced with a new support.set_match_tests() function, so match_test() doesn't have to check each time if patterns were modified. * Rewrite match_test(): use different code paths depending on the kind of patterns for best performances. Co-Authored-By: Serhiy Storchaka <[email protected]> (cherry picked from commit 803ddd8) * bpo-31324: Fix test.support.set_match_tests(None) (#4505) (cherry picked from commit bb11c3c)

* bpo-31324: Optimize support._match_test() (#4421) * Rename support._match_test() to support.match_test(): make it public * Remove support.match_tests global variable. It is replaced with a new support.set_match_tests() function, so match_test() doesn't have to check each time if patterns were modified. * Rewrite match_test(): use different code paths depending on the kind of patterns for best performances. Co-Authored-By: Serhiy Storchaka <[email protected]> (cherry picked from commit 803ddd8) * bpo-31324: Fix test.support.set_match_tests(None) (#4505) (cherry picked from commit bb11c3c) (cherry picked from commit 70b2f87)

the-knights-who-say-ni added the CLA signed label Nov 16, 2017

bedevere-bot added the awaiting merge label Nov 16, 2017

vstinner mentioned this pull request Nov 16, 2017

bpo-31324: Optimize support._match_test(). #4420

Closed

vstinner added the skip news label Nov 16, 2017

serhiy-storchaka reviewed Nov 16, 2017

View reviewed changes

serhiy-storchaka reviewed Nov 21, 2017

View reviewed changes

vstinner added 2 commits November 21, 2017 15:04

set_match_tests() does nothing if patterns don't change

bdec1ff

Add more tests

7b2df80

Use also support.swap_attr().

Address Serhiy's comments

ac956e3

serhiy-storchaka reviewed Nov 21, 2017

View reviewed changes

set_match_tests() copies patterns

c28b3c0

vstinner merged commit 803ddd8 into python:master Nov 21, 2017

bedevere-bot removed the awaiting merge label Nov 21, 2017

vstinner deleted the set_match_tests branch November 21, 2017 23:34

vstinner mentioned this pull request Nov 23, 2017

[2.7] bpo-31324: Optimize support._match_test() (#4523) #4524

Merged

Uh oh!

Conversation

vstinner commented Nov 16, 2017 • edited by serhiy-storchaka Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 21, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vstinner commented Nov 16, 2017 •

edited by serhiy-storchaka

Loading

vstinner commented Nov 16, 2017 •

edited

Loading