GeezerVison

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I checked correctness of the return expression replacement, using test_cases above, with

for s in test_cases:
    (pat(s) is not None) is (s.strip() in ('', '#'))

and all are True.

I checked correctness of the return expression replacement, using test_cases above, with

Ok, thank you, but this test_cases is too small a sample and therefore I have supplemented my script with the following checks:

import string
import random
from itertools import product

# First Checks:
for case in test_cases:
    assert is_line_junk_regex(case) == is_line_junk_no_regex(case), f"Inconsistent behavior for {repr(case)}"

def random_string(length=10):
    chars = string.printable + " " * 5 + "#" * 5
    return "".join(random.choice(chars) for _ in range(length))

# Second Checks:
for _ in range(10000):
    case = random_string(random.randint(0, 20))
    assert is_line_junk_regex(case) == is_line_junk_no_regex(case), f"Inconsistent behavior for {repr(case)}"

# Third Checks:
chars = " #\tabc"
for length in range(6):
    for case in map("".join, product(chars, repeat=length)):
        assert is_line_junk_regex(case) == is_line_junk_no_regex(case), f"Inconsistent behavior for {repr(case)}"

Then I checked the tests in Lib/test/test_difflib.py and everything finished well 👍

I have made the requested changes; please review again

Thanks for making the requested changes!

@terryjreedy, @AA-Turner: please review the changes made to this pull request.

Since we're improving performance, we don't want to recompute line.strip()

Sorry, I suspected this but hoped that there was a cache mechanism here :)

I checked correctness of the return expression replacement, using test_cases above, with

for s in test_cases:
    (pat(s) is not None) is (s.strip() in ('', '#'))

and all are True.

I am going to be neutral an whether the basic change should be merged. If it is, I personally think the 'pat' parameter can be considered to be private and eliminated. But I would defer the back-compatibility to others, include 'encukou'. The only documented use of the function, marked as a constant, is to be passed as an argument. The argument is always passed on to SequenceMatcher, which calls it once with 1 argument, which would be a line.

@serhiy-storchaka

In any case, the relative performance depends on the input -- for a long string with a single space character at one end (e.g. 'x'*10000+' ') the non-regex version is more than 10 times slower than the regex version. For length 100000 the difference is more than 100, and for length 1000000 -- more than 1000.

I tested your cases but didn't get the result you expected (maybe I did something wrong?)
I ran next script (accidentally mixed up the dividend and divisor, but it doesn't matter):

import re
import timeit

def is_line_junk_regex(line, pat=re.compile(r"\s*(?:#\s*)?$").match):
    return pat(line) is not None

def is_line_junk_no_regex(line):
    return line.strip() == "" or line.lstrip().rstrip() == "#"

def benchmark(func, cases, n=1_000):
    return timeit.timeit(lambda: [func(line) for line in cases], number=n)

lengths = [10_000, 100_000, 1_000_000]
for length in lengths:
    test_string = "x" * length + " "
    regex_time = benchmark(is_line_junk_regex, test_string)
    no_regex_time = benchmark(is_line_junk_no_regex, test_string)

    print(f"String length: {length}")
    print(f"Regex version: {regex_time:.6f} seconds")
    print(f"Non-regex version: {no_regex_time:.6f} seconds")
    print(f"Speedup: {no_regex_time / regex_time:.2f}x\n")

and got next results:

$ ./python -B bench.py
String length: 10000
Regex version: 4.220341 seconds
Non-regex version: 1.983180 seconds
Speedup: 0.47x

String length: 100000
Regex version: 42.698992 seconds
Non-regex version: 20.897327 seconds
Speedup: 0.49x

String length: 1000000
Regex version: 516.844790 seconds
Non-regex version: 262.632697 seconds
Speedup: 0.51x

Aren't you iterating over a single character instead of a full line:

[func(line) for line in cases]

You should pass [test_string] instead of just test_string (sorry if I'm wrong, I'm tired)

I was the one who originally reqested restoring pat, but I do find Terry's argument compelling, and I wouldn't oppose removing pat.

I think we should restore this PR to the simpler previous version (the differences micro-benchmarks between string comparisons are so small they will be swallowed by noise). The class-level re import should be resolved before merge, though.

A

@tim-one Do you have a reason to want to keep the regex?

Can't say I much care, but I don't get it. What's the point? It's dubious on the face of it to build an entirely new string object just to do simple lexical analysis. If this is some "micro-optimizztion" thing, instead of

        stripped = line.strip()
        return stripped == '' or stripped == '#'

try

        return line.strip() in '#' # empty or single '#'

?

Can't say I much care, but I don't get it. What's the point? It's dubious on the face of it to build an entirely new string object just to do simple lexical analysis. If this is some "micro-optimizztion" thing, instead of

        stripped = line.strip()

        return stripped == '' or stripped == '#'

try

        return line.strip() in '#' # empty or single '#'

?

that's how it was originally :)

I'll probably return it so as not to overcomplicate things

@rhettinger

@tim-one Do you have a reason to want to keep the regex?

Tim doesn't seem to mind

I don't know who I can ping but it seems like there are more suggestions