Skip to content
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

optimize get_for_dialect, get_db_field_types #1863

Merged
merged 10 commits into from
Jan 28, 2025

Conversation

markus-96
Copy link
Contributor

Description

its not much, but could improve performance in certain use cases.

Motivation and Context

saw it

How Has This Been Tested?

make ci
benchmarked on my laptop, see #1862

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added the changelog accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link

codspeed-hq bot commented Jan 23, 2025

CodSpeed Performance Report

Merging #1863 will improve performances by ×10

Comparing markus-96:optimize-Field-methods (a54fcb8) with develop (95f9467)

Summary

⚡ 3 improvements
✅ 9 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
test_expressions_count 973.8 µs 858.8 µs +13.4%
test_expressions_f 847.7 µs 764.8 µs +10.84%
test_field_attribute_lookup_get_for_dialect 624.5 µs 59.7 µs ×10

@markus-96
Copy link
Contributor Author

will fix coding style issues if #1862 is merged to see if it actually impacts performance...

@markus-96
Copy link
Contributor Author

the following shows what this is all about and how I verified a performance optimization:

code:
from typing import Any

from tortoise.fields import Field


class MyField:
    _get_dialects = Field._get_dialects  # lets link this to MyField

    @property
    def MY_PROPERTY(self):
        return f"hi from {self.__class__.__name__}!"

    OTHER_PROPERTY = "something else"

    class _db_property:
        def __init__(self, field: "Field"):
            self.field = field

        @property
        def MY_PROPERTY(self):
            return f"hi from {self.__class__.__name__} of {self.field.__class__.__name__}!"

    class _db_cls_attribute:
        MY_PROPERTY = "cls_attribute"

    def get_for_dialect(self, dialect: str, key: str) -> Any:
        """
        the optimized get_for_dialect method
        """
        cls = getattr(self, f"_db_{dialect}", self)
        value = getattr(cls, key, getattr(self, key, None))
        if value is None:
            return None
        elif isinstance(value, property) and isinstance(cls, type):
            return getattr(cls(self), key)
        return value

    def get_for_dialect_old(self, dialect: str, key: str) -> Any:
        """
        the current implementation of get_for_dialect
        """
        dialect_data = self._get_dialects().get(dialect, {})
        return dialect_data.get(key, getattr(self, key, None))


def test_get_for_dialect():
    """lets test if it functions the same way"""
    assert MyField().get_for_dialect("property", "MY_PROPERTY") == MyField().get_for_dialect_old("property", "MY_PROPERTY")
    assert MyField().get_for_dialect("non_existent_dialect", "MY_PROPERTY") == MyField().get_for_dialect_old("non_existent_dialect", "MY_PROPERTY")
    assert MyField().get_for_dialect("cls_attribute", "MY_PROPERTY") == MyField().get_for_dialect_old("cls_attribute", "MY_PROPERTY")
    assert MyField().get_for_dialect("property", "OTHER_PROPERTY") == MyField().get_for_dialect_old("property", "OTHER_PROPERTY")
    assert MyField().get_for_dialect("property", "MY_PROPERTY") == MyField().get_for_dialect_old("property", "MY_PROPERTY")


def test_get_for_dialect_benchmark(benchmark):
    """lets benchmark the new implementation"""
    @benchmark
    def bench():
        MyField().get_for_dialect("property", "MY_PROPERTY")
        MyField().get_for_dialect("postgres", "MY_PROPERTY")
        MyField().get_for_dialect("cls_attribute", "MY_PROPERTY")
        MyField().get_for_dialect("property", "OTHER_PROPERTY")
        MyField().get_for_dialect("property", "MY_PROPERTY")


def test_get_for_dialect_old_benchmark(benchmark):
    """lets benchmark the old implementation"""
    @benchmark
    def bench():
        MyField().get_for_dialect_old("property", "MY_PROPERTY")
        MyField().get_for_dialect_old("postgres", "MY_PROPERTY")
        MyField().get_for_dialect_old("cls_attribute", "MY_PROPERTY")
        MyField().get_for_dialect_old("property", "OTHER_PROPERTY")
        MyField().get_for_dialect_old("property", "MY_PROPERTY")
console:
~: pytest .\t.py --codspeed
                                   Benchmark Results
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━┓
┃                          Benchmark ┃ Time (best) ┃ Rel. StdDev ┃ Run time ┃   Iters ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━┩
│     test_get_for_dialect_benchmark │     3,200ns │       51.1% │    3.00s │ 827,418 │
│ test_get_for_dialect_old_benchmark │    75,400ns │       27.5% │    2.92s │  33,557 │
└────────────────────────────────────┴─────────────┴─────────────┴──────────┴─────────┘

will fix coding style issues if #1862 is merged to see if it actually impacts performance...

should I check isinstance(cls, type), like in this comment above, or just add # type: ignore?

@henadzit
Copy link
Contributor

@markus-96 looking forward to seeing the benchmark results!

tortoise/fields/base.py Outdated Show resolved Hide resolved
tortoise/fields/base.py Outdated Show resolved Hide resolved
tortoise/fields/base.py Outdated Show resolved Hide resolved
tortoise/fields/base.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jan 28, 2025

Pull Request Test Coverage Report for Build 13011947162

Details

  • 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
  • 14 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 89.239%

Files with Coverage Reduction New Missed Lines %
tortoise/fields/base.py 14 81.93%
Totals Coverage Status
Change from base Build 13008882482: -0.2%
Covered Lines: 6488
Relevant Lines: 7087

💛 - Coveralls

@markus-96
Copy link
Contributor Author

completely rewritten, but now it is not that fast anymore because the AttributeError will be raised many, many times and therefore needs to catched. But Codacy is happy now, at least.

@henadzit
Copy link
Contributor

completely rewritten, but now it is not that fast anymore because the AttributeError will be raised many, many times and therefore needs to catched. But Codacy is happy now, at least.

Thanks! It's still an enormous speedup and the code looks very clean. I think we should merge it if CI passes.

@henadzit henadzit merged commit 960b1c1 into tortoise:develop Jan 28, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants