GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-130167: Improve speed of `ftplib.parse150` by replacing `re` by donbarbos · Pull Request #130243 · python/cpython

Conversation

@donbarbos

@donbarbos donbarbos commented Feb 18, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I also suggest adding tests for this function because they are missing in the Lib/test/test_ftplib.py file.
I wrote tests (would be in next comment) and benchmarks (results here)

parse150_bench.py:

from Lib.ftplib import error_reply
import timeit

# Regex version
_150_re = None
def parse150_re(resp):
    if resp[:3] != "150":
        raise error_reply(resp)
    global _150_re
    if _150_re is None:
        import re
        _150_re = re.compile(r"150 .* \((\d+) bytes\)", re.IGNORECASE | re.ASCII)
    m = _150_re.match(resp)
    if not m:
        return None
    return int(m.group(1))

# No regex version
def parse150(resp):
    if not resp.startswith("150"):
        raise error_reply(resp)

    start = resp.find("(")
    end = resp.find(" bytes)")

    if start == -1 or end == -1 or start >= end:
        return None

    try:
        return int(resp[start + 1 : end])
    except ValueError:
        return None

test_cases = [
    "150 Opening BINARY mode data connection (4096 bytes)",
    "150 Transfer starting (32768 bytes)",
    "150 Data connection accepted (131072 bytes)",
    "150 Some random message without bytes",
    "150 Large file transfer (987654321 bytes)",
    "150 Small file (1 bytes)",
    "150 Transfer starting (99999999 bytes)",
    "150 Weird case (42 bytes) with extra text",
]

for case in test_cases:
    regex_time = timeit.timeit(lambda: parse150_re(case), number=1_000_000)
    no_regex_time = timeit.timeit(lambda: parse150(case), number=1_000_000)
    print(f"Test case: {case}")
    print(f"  Regex version: {regex_time:.6f} sec")
    print(f"  No regex version: {no_regex_time:.6f} sec")

Results for different cases (from x1.82 to x1.92 as fast):

$ ./python -B parse150_bench.py
Test case: 150 Opening BINARY mode data connection (4096 bytes)
  Regex version: 1.147499 sec
  No regex version: 0.630863 sec
Test case: 150 Transfer starting (32768 bytes)
  Regex version: 1.113087 sec
  No regex version: 0.610494 sec
Test case: 150 Data connection accepted (131072 bytes)
  Regex version: 1.140198 sec
  No regex version: 0.616784 sec
Test case: 150 Some random message without bytes
  Regex version: 0.643958 sec
  No regex version: 0.302508 sec
Test case: 150 Large file transfer (987654321 bytes)
  Regex version: 1.160474 sec
  No regex version: 0.605776 sec
Test case: 150 Small file (1 bytes)
  Regex version: 1.005945 sec
  No regex version: 0.522877 sec
Test case: 150 Transfer starting (99999999 bytes)
  Regex version: 1.147927 sec
  No regex version: 0.616777 sec
Test case: 150 Weird case (42 bytes) with extra text
  Regex version: 1.140925 sec
  No regex version: 0.592486 sec
  • Issue: Improve speed of stdlib functions by replacing re uses #130167

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
donbarbos added 3 commits February 18, 2025 04:55
@donbarbos
Improve speed of ftplib.parse150 by replacing re
4055dbe
@donbarbos
Add blurb NEWS.d entry
043de73
@donbarbos
Correct blurb entry file
8ad9c5e
@bedevere-app bedevere-app Bot added the awaiting review label Feb 18, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Feb 18, 2025
Improve speed of stdlib functions by replacing re uses #130167
Open
@donbarbos

donbarbos commented Feb 18, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

my tests passed completely. this is a continuation of the previous file:

import unittest

class TestParse150(unittest.TestCase):
    def test_valid_cases(self):
        resp = "150 Opening BINARY mode data connection (1024 bytes)"
        self.assertEqual(parse150(resp), parse150_re(resp), 1024)

        resp = "150 Opening BINARY mode data connection (2048 bytes)"
        self.assertEqual(parse150(resp), parse150_re(resp), 2048)

        resp = "150 Transfer starting (8192 bytes)"
        self.assertEqual(parse150(resp), parse150_re(resp), 8192)

        resp = "150 Data connection accepted (65536 bytes)"
        self.assertEqual(parse150(resp), parse150_re(resp), 65536)

        resp = "150 Data connection accepted (65536 BYTES)"
        self.assertEqual(parse150(resp), parse150_re(resp), 65536)

    def test_invalid_cases(self):
        resp = "150 Opening connection"
        self.assertEqual(parse150(resp), parse150_re(resp), None)

        resp = "150 (not a number bytes)"
        self.assertEqual(parse150(resp), parse150_re(resp), None)

        resp = "150 (1234 bytes"
        self.assertEqual(parse150(resp), parse150_re(resp), None)

        resp = "150 (5678 bytes"
        self.assertEqual(parse150(resp), parse150_re(resp), None)

        resp = "150  () bytes)"
        self.assertEqual(parse150(resp), parse150_re(resp), None)

        resp = "200 OK"
        self.assertRaises(error_reply, parse150, resp)
        self.assertRaises(error_reply, parse150_re, resp)

if __name__ == "__main__":
    unittest.main()
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eendebakpt
eendebakpt reviewed Feb 18, 2025
View reviewed changes
Comment thread Lib/ftplib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@donbarbos
Ignore case while finding
e5dd027
@eendebakpt

eendebakpt commented Feb 18, 2025

Copy link
Copy Markdown
Contributor

This improves the performance of parse150 for some cases. But is the performance a bottleneck? It might be performance of ftplib is limited by the actual transfers.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@donbarbos
Update NEWS.d entry markup
db0a29a
@rhettinger

rhettinger commented Feb 18, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I'm going to decline this one. It is ancient, well-exercised, stable code that is not much used anymore. It really isn't worth the code churn or the risk that the string method approach isn't exactly equivalent (i.e. lower() vs casefold()).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhettinger rhettinger closed this Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eendebakpt eendebakpt eendebakpt left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@donbarbos @eendebakpt @rhettinger
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.