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

Performance issues X certificates #270

Open
KonstantAnxiety opened this issue Jan 31, 2024 · 4 comments
Open

Performance issues X certificates #270

KonstantAnxiety opened this issue Jan 31, 2024 · 4 comments

Comments

@KonstantAnxiety
Copy link
Collaborator

KonstantAnxiety commented Jan 31, 2024

{
    "DatasetFieldsGet.create-ssl-context": {
        "_cases": "0_2044_0-rc_1 -> 0_2045_0-rc1 -> 0_2046_0-rc1 -> py312",
        "avg": "0.005005s -> 0.038518s (+33ms) -> 0.040204s (+1ms) -> 0.041159s (+0ms) [total: +36ms]",
        "avg_sum": "0.010010s -> 0.077035s (+67ms) -> 0.040204s (-36ms) -> 0.041158s (+0ms) [total: +31ms]",
        "median": "0.004900s -> 0.038000s (+33ms) -> 0.039800s (+1ms) -> 0.040550s (+0ms) [total: +35ms]"
    },
    "DatasetFieldsGet.us-client-request-session": {
        "_cases": "0_2044_0-rc_1 -> 0_2045_0-rc1 -> 0_2046_0-rc1 -> py312",
        "avg": "0.011083s -> 0.045357s (+34ms) -> 0.040809s (-4ms) -> 0.041656s (+0ms) [total: +30ms]",
        "avg_sum": "0.022166s -> 0.090713s (+68ms) -> 0.040809s (-49ms) -> 0.041657s (+0ms) [total: +19ms]",
        "median": "0.011200s -> 0.045300s (+34ms) -> 0.040400s (-4ms) -> 0.041100s (+0ms) [total: +29ms]"
    },
    "DatasetFieldsGet.us-fetch-entity": {
        "_cases": "0_2044_0-rc_1 -> 0_2045_0-rc1 -> 0_2046_0-rc1 -> py312",
        "avg": "0.018098s -> 0.053239s (+35ms) -> 0.011738s (-41ms) -> 0.010666s (-1ms) [total: -7ms]",
        "avg_sum": "0.036197s -> 0.106479s (+70ms) -> 0.023476s (-83ms) -> 0.021331s (-2ms) [total: -14ms]",
        "median": "0.020950s -> 0.054700s (+33ms) -> 0.015400s (-39ms) -> 0.013750s (-1ms) [total: -7ms]"
    },
    "DatasetVersionResult.create-ssl-context": {
        "_cases": "0_2044_0-rc_1 -> 0_2045_0-rc1 -> 0_2046_0-rc1 -> py312",
        "avg": "0.004906s -> 0.038715s (+33ms) -> 0.039965s (+1ms) -> 0.040819s (+0ms) [total: +35ms]",
        "avg_sum": "0.009812s -> 0.077430s (+67ms) -> 0.039965s (-37ms) -> 0.040818s (+0ms) [total: +31ms]",
        "median": "0.004800s -> 0.038100s (+33ms) -> 0.039300s (+1ms) -> 0.040200s (+0ms) [total: +35ms]"
    },
    "DatasetVersionResult.dataset-prepare": {
        "_cases": "0_2044_0-rc_1 -> 0_2045_0-rc1 -> 0_2046_0-rc1 -> py312",
        "avg": "0.009081s -> 0.009295s (+0ms) -> 0.009192s (+0ms) -> 0.006944s (-2ms) [total: -2ms]",
        "avg_sum": "0.009081s -> 0.009294s (+0ms) -> 0.009193s (+0ms) -> 0.006944s (-2ms) [total: -2ms]",
        "median": "0.008450s -> 0.008600s (+0ms) -> 0.008400s (+0ms) -> 0.006800s (-1ms) [total: -1ms]"
    },
    "DatasetVersionResult.ds-result-full": {
        "_cases": "0_2044_0-rc_1 -> 0_2045_0-rc1 -> 0_2046_0-rc1 -> py312",
        "avg": "0.096509s -> 0.166221s (+69ms) -> 0.084498s (-81ms) -> 0.074960s (-9ms) [total: -21ms]",
        "avg_sum": "0.096509s -> 0.166221s (+69ms) -> 0.084498s (-81ms) -> 0.074961s (-9ms) [total: -21ms]",
        "median": "0.092000s -> 0.163200s (+71ms) -> 0.079650s (-83ms) -> 0.072600s (-7ms) [total: -19ms]"
    },
    "DatasetVersionResult.us-client-request-session": {
        "_cases": "0_2044_0-rc_1 -> 0_2045_0-rc1 -> 0_2046_0-rc1 -> py312",
        "avg": "0.010821s -> 0.045417s (+34ms) -> 0.040456s (-4ms) -> 0.041253s (+0ms) [total: +30ms]",
        "avg_sum": "0.021641s -> 0.090834s (+69ms) -> 0.040456s (-50ms) -> 0.041254s (+0ms) [total: +19ms]",
        "median": "0.010800s -> 0.045100s (+34ms) -> 0.039800s (-5ms) -> 0.040600s (+0ms) [total: +29ms]"
    },
    "DatasetVersionResult.us-fetch-entity": {
        "_cases": "0_2044_0-rc_1 -> 0_2045_0-rc1 -> 0_2046_0-rc1 -> py312",
        "avg": "0.018511s -> 0.052082s (+33ms) -> 0.011673s (-40ms) -> 0.010302s (-1ms) [total: -8ms]",
        "avg_sum": "0.037022s -> 0.104165s (+67ms) -> 0.023346s (-80ms) -> 0.020605s (-2ms) [total: -16ms]",
        "median": "0.019900s -> 0.053700s (+33ms) -> 0.011650s (-42ms) -> 0.012750s (+1ms) [total: -7ms]"
    },
    "DatasetVersionResult.validator-apply-action-batch": {
        "_cases": "0_2044_0-rc_1 -> 0_2045_0-rc1 -> 0_2046_0-rc1 -> py312",
        "avg": "0.006097s -> 0.006997s (+0ms) -> 0.006931s (+0ms) -> 0.005044s (-1ms) [total: -1ms]",
        "avg_sum": "0.006098s -> 0.006997s (+0ms) -> 0.006931s (+0ms) -> 0.005043s (-1ms) [total: -1ms]",
        "median": "0.006100s -> 0.006250s (+0ms) -> 0.006200s (+0ms) -> 0.005000s (-1ms) [total: -1ms]"
    },
    "TOTAL_REQUEST_TIME": {
        "_cases": "0_2044_0-rc_1 -> 0_2045_0-rc1 -> 0_2046_0-rc1 -> py312",
        "avg": "0.186652s -> 0.396197s (+209ms) -> 0.231896s (-164ms) -> 0.220877s (-11ms) [total: +34ms]",
        "avg_sum": "0.186652s -> 0.396197s (+209ms) -> 0.231896s (-164ms) -> 0.220877s (-11ms) [total: +34ms]",
        "median": "0.179142s -> 0.389798s (+210ms) -> 0.225339s (-164ms) -> 0.216757s (-8ms) [total: +37ms]"
    }
}
@thenno
Copy link
Contributor

thenno commented Jan 31, 2024

Please take a look at a more illustrative example of reproducing the issue with OpenSSL.
Laptop: macbook pro 13 m1 (1th generation), ssd disk

https://gist.github.com/thenno/f9282e24dbf03e15d4a75d2bf33331a4

Some results
This issue is not connected with #233 (same timings in run_cadata and run_file). This issue affects only network disks, not local ssd.

Ubuntu 20.04 (OpenSSL 1.1.1f, python 3.8.10) -> ~3ms per ssl context
Ubuntu 22.04 (OpenSSL 3.0.2, python 3.10.12) -> ~40ms
Ubuntu 24.04 dev (OpenSSL 3.0.10, python 3.11.7) -> ~15ms

@KonstantAnxiety
Copy link
Collaborator Author

@thenno

I never said that this issue is connected to #233. Moreover, the fixes made for #233 also help the situation in this issue – the fewer times create_default_context is called, the better, regardless of whether cafile or cadata is used.

I appreciate the effort of putting this experiment together, but I'm failing to see if you are suggesting something specific or not. Your measurements, as I see them, look like a further confirmation of the performance degradation introduced with Ubuntu 22.04 (#267).

@thenno
Copy link
Contributor

thenno commented Feb 1, 2024

@KonstantAnxiety

We indeed think in the same direction, my example just illustrates the current issue in another way and its dependence at least on the version of Ubuntu and OpenSSL, but not on the method of creating context. I hope that my test will help you with diagnostics and reproduction.

But so far, I can't understand how the changes in #233 improve the situation. As far as I can see, the SSLContext is created exactly in the same places as before, the creating method also does not significantly affect the runtime. Because of this, I don't understand how there could be fewer calls to create_default_context.

The situation with OpenSSL is quite unpleasant, and I don't have a really good solution either.

Some options are:

  1. Passing SSLContext through ServiceRegistry is a bad idea, as we have at least two types of contexts (ssl for aiohttp, and grpc)
  2. Use a more recent version of OpenSSL, but as far as I can see, they are not available in either standard repositories or PPA.
  3. Add caching (standart lry cache, for example) of the SSLContext to each client. 300kb could save ~40ms of CPU, I think it's a great trade-off. I find this method a bit messy, but I don't see how it can affect the further development of the project. I hope OpenSSL team will fix it and this workaround can eventually be removed (just remove lru cache).

What do you think about just adding some caches?
For example

def _make_session(self) -> aiohttp.ClientSession:


from functools import lru_cache

@lru_cache
def make_aiohttp_ssl_context(ca_data: bytes) -> ssl.SSLContext:
    return ssl.create_default_context(ca_data.decode("ascii"))

class BIAioHTTPClient
    def _make_session(self) -> aiohttp.ClientSession:
        ssl_context = make_aiohttp_ssl_context(self._ca_data)
        return aiohttp.ClientSession(
            cookies=self.cookies,
            headers=self.headers,
            connector=aiohttp.TCPConnector(
                ssl_context=ssl_context,
            ),
        )

@KonstantAnxiety
Copy link
Collaborator Author

@thenno

As far as I can see, the SSLContext is created exactly in the same places as before

My bad, this was actually a side-effect of your 1st cert PR. Here is the part that make the amount of times SSLContext is created drop twofold – previously BIAioHTTPClient was instantiated on every request, leading to this call in a factory, no matter the session passed inside.

What do you think about just adding some caches?

I'll run some tests and come back.

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

No branches or pull requests

2 participants