Add a subclass to optimize Versions

[grden]

This PR adds class CachedVersion, a subclass of packaging.version.Version which caches its hash and string representations to avoid re-computation at each comparison.

This closes #73.

[ryanking13]

Thanks, @grden for picking up this issue.

As this is related to the performance improvement, I think it would be good to have some simple benchmark to test if it really improves the performance.

It is not easy to test a real-like scenario as it depends on network conditions, etc. So I think writing a short script that simply compares the performance of Version and CachedVersion would be enough, without doing any network calls.

Could you write some benchmark script, run it in both native and Pyodide environment and see if you can observe performance gains?

[ryanking13]

This file needs some unittest

[grden]

I've added tests in a separate file for now. Any feedback on the tests would be much appreciated, since I'm not too famililar with testing.

[hoodmane]

Cc @henryiii @brettcannon does this optimization make sense to you? Would it make sense to contribute something like this upstream? I don't think there's anything special about our use case.

[hoodmane]

Maybe we could compute these lazily rather than on construction?

[ryanking13]

+1

[brettcannon]

I don't see the hash being a performance concern as it comes from a tuple which itself will cache the results.

https://github.com/pypa/packaging/blob/258202ed7f796bdb8a65252a66c3fbd3e69e97f6/src/packaging/version.py#L72-L73

[grden]

Below is the result when I tested performance in native environment.

#!/usr/bin/env python3
import timeit
import random
import json
from functools import partial

from micropip._vendored.packaging.src.packaging.version import Version
from micropip._cached_version import CachedVersion

def benchmark_original_version(version_strings, iterations):
    versions = [Version(vs) for vs in version_strings]
    
    for _ in range(iterations):
        releases = {v: [f"file_{v}"] for v in versions}
        releases_sorted = dict(sorted(releases.items()))    
        
        for _ in range(100):
            lookup_version = random.choice(versions)
            _ = releases_sorted.get(lookup_version)

def benchmark_cached_version(version_strings, iterations):
    versions = [CachedVersion(vs) for vs in version_strings]
    
    for _ in range(iterations):
        releases = {v: [f"file_{v}"] for v in versions}
        releases_sorted = dict(sorted(releases.items()))
        
        for _ in range(100):
            lookup_version = random.choice(versions)
            _ = releases_sorted.get(lookup_version)

if __name__ == "__main__":
    package_files = ["snowballstemmer_simple.json", "pytest_simple.json"]
    iteration_counts = [1, 20, 100]
    
    for pkg_file in package_files:
        with open(f"tests/test_data/pypi_response/{pkg_file}", "rb") as f:
            data = json.loads(f.read())
            current_versions = data.get("versions", [])

        print(f"\nTesting package '{pkg_file}' ({len(current_versions)} versions)")

        for iterations in iteration_counts:
            print(f"\nfor {iterations} iteration(s):")
            
            original_runner = partial(benchmark_original_version, current_versions, iterations)
            cached_runner = partial(benchmark_cached_version, current_versions, iterations)
            
            original_time = timeit.timeit(original_runner, number=10)
            cached_time = timeit.timeit(cached_runner, number=10)
            
            improvement_factor = original_time / cached_time if cached_time > 0 else float('inf')
            
            print(f"Original Version class: {original_time:.4f} seconds")
            print(f"CachedVersion class:    {cached_time:.4f} seconds")
            
            if improvement_factor > 1:
                print(f"=> CachedVersion is {improvement_factor:.2f} times faster")
            else:
                print(f"=> CachedVersion is {(1/improvement_factor if improvement_factor > 0 else 0):.2f} times slower")

        print("-" * 50)

Testing package 'snowballstemmer_simple.json' (10 versions)

for 1 iteration(s):
Original Version class: 0.0010 seconds
CachedVersion class:    0.0007 seconds
=> CachedVersion is 1.46 times faster

for 20 iteration(s):
Original Version class: 0.0156 seconds
CachedVersion class:    0.0058 seconds
=> CachedVersion is 2.69 times faster

for 100 iteration(s):
Original Version class: 0.0738 seconds
CachedVersion class:    0.0262 seconds
=> CachedVersion is 2.82 times faster
--------------------------------------------------

Testing package 'pytest_simple.json' (162 versions)

for 1 iteration(s):
Original Version class: 0.0073 seconds
CachedVersion class:    0.0075 seconds
=> CachedVersion is 1.03 times slower

for 20 iteration(s):
Original Version class: 0.0691 seconds
CachedVersion class:    0.0246 seconds
=> CachedVersion is 2.81 times faster

for 100 iteration(s):
Original Version class: 0.3366 seconds
CachedVersion class:    0.1015 seconds
=> CachedVersion is 3.31 times faster
--------------------------------------------------

The change shows a performance gain especially for large number of versions, but I’m not sure if the benefit would outweigh the added complexity and memory cost. I'd really appreciate any feedback.

[ryanking13]

+1

[ryanking13]

parse_wheel_filename will always return Version object. Changing it to str and converting it to CachedVersion again is wasteful. How about creating a helper function, something like

CachedVersion.from_version(version_obj)

[grden]

Great idea, thank you!

[ryanking13]

Thanks for writing the benchmark script @grden.

I left some comments, but overall it looks good. Let's wait for inputs from pypa folks as well.

---

The comment below is not directly related to your PR, but some ideas about optimizing this part more. Feel free to take a look if you are interested (no action need for this PR).

• Cashing the hash would be helpful when randomly accessing the dictionary that uses the version key, but one of the major operations against the Version object, I think, is <= or >= operations, which are not optimized by cashing the hash.

• Also, one bottleneck that I noticed when I was writing the original issue (Optimize the speed of parsing and comparing Versions #73) was that initializing the Version instance is quite heavy. So maybe we can think of making a pool that returns a shared Version instance... maybe something like

global_version_pool: dict[str, Version] = {}

def parse_version(version: str) -> Version:
    global global_version_pool

    if version in global_version_pool:
        return global_version_pool[version]

    new_version = Version(version)
    global_version_pool[version] = new_version
    return new_version

[brettcannon]

Would it make sense to contribute something like this upstream?

Maybe. I don't think the hashing needs caching (see my comment). The string stuff might if you can make it lazy as I don't remember the string being required upfront. But part of the trick is the class is mutable (although I think all public attributes are properties without setters), so probably some sanity check against the hash or something might be needed to invalidate things. But this is all stuff we could talk about in a PR.

[grden]

Thank you for the great feedback.

I don't see the hash being a performance concern as it comes from a tuple which itself will cache the results.

This explains why I saw a significant speedup from string caching but less improvement from hash caching on previous benchmarks. I've updated the class to use lazy caching, and for __str__ only.

Regarding the sanity check, my thinking was that since the class is treated as immutable in practice(with no public setters), adding a check in every __str__ call might cause performance overhead that goes against the goal of optimization. It would be really appreciated to hear thoughts on this.

Below is the updated benchmark that focuses on the performance of str() calls, for reference.

...

def benchmark_original_version(version_strings, iterations):
    versions = [Version(vs) for vs in version_strings]
    
    for _ in range(iterations):
        for v in versions:
            _ = str(v)

def benchmark_cached_version(version_strings, iterations):
    versions = [CachedVersion(vs) for vs in version_strings]
    
    for _ in range(iterations):
        for v in versions:
            _ = str(v)
...

Testing package 'snowballstemmer_simple.json' (10 versions)

for 1 iteration(s):
Original Version class: 0.0009 seconds
CachedVersion class:    0.0004 seconds
=> CachedVersion is 2.12 times faster

for 20 iteration(s):
Original Version class: 0.0018 seconds
CachedVersion class:    0.0005 seconds
=> CachedVersion is 3.90 times faster

for 100 iteration(s):
Original Version class: 0.0128 seconds
CachedVersion class:    0.0021 seconds
=> CachedVersion is 6.09 times faster
--------------------------------------------------

Testing package 'pytest_simple.json' (162 versions)

for 1 iteration(s):
Original Version class: 0.0059 seconds
CachedVersion class:    0.0057 seconds
=> CachedVersion is 1.03 times faster

for 20 iteration(s):
Original Version class: 0.0240 seconds
CachedVersion class:    0.0069 seconds
=> CachedVersion is 3.46 times faster

for 100 iteration(s):
Original Version class: 0.1011 seconds
CachedVersion class:    0.0124 seconds
=> CachedVersion is 8.14 times faster
--------------------------------------------------

[ryanking13]

This explains why I saw a significant speedup from string caching but less improvement from hash caching on previous benchmarks. I've updated the class to use lazy caching, and for str only.

Hmm, but I don't think we call str against the Version object in package_index.py. So I am not sure caching str is that worth it for us. I mean, it was me that originally suggested that idea in the upstream issue, but maybe it is not very worth it.

[grden]

Actually, you're right about str calls not causing bottleneck in package_index.py or anywhere else. Thanks for pointing this out. I'll close this.

[ryanking13]

@grden, sorry for the time you took to work on this. If you are interested in optimizing micropip in any other parts. Please feel free to take a look. I think there would be some lot of low-hanging fruit in micropip.

[grden]

No worries! Thank you for taking time into this.