Opened 10 months ago

Last modified 4 months ago

#9138 enhancement new

Create a new context factory that caches the IOpenSSLClientConnectionCreator

Reported by: Jason Litzinger Owned by:
Priority: normal Milestone:
Component: core Keywords: review
Cc: Branch:


The context factory provided to twisted.web.client.Agent is responsible for implementing the twisted.web.iweb.IPolicyForHTTPS and providing an API to obtaina connection creator that can validate certificates and hostnames for a specific hostname and port. The creation of this object can, on certain hardware (e.g.ARM Cortex-A5) introduce system load that is unnecessary if requests are made to the same hostname more than once.

This enhancement proposes the creation of a context factory that maintains a simple LRU cache of the last cacheSize hostnames containing the connection creator that can used to create/verify connections.

The original discussion of this feature, and the problem that inspired it is documented here:

Change History (6)

comment:1 Changed 10 months ago by Jason Litzinger

WIP on this can be found here:

Though please note that, until I submit a PR for review this branch, and all commits may be deleted, recreated, rebased, and otherwise mucked about with. If anyone happens to be using it and would rather this not happen please let me know.

comment:2 Changed 10 months ago by Jason Litzinger

Keywords: review added

The pull request for initial review of this ticket is located:

The changes adds a new IPolicyForHTTPS that supports caching previously instantiated IOpenSSLClientConnectionCreator objects for re-use in subsequent calls to creatorForNetloc().

To facilitate testing, I also moved the CustomOpenSSLTrustRoot out of the test method it was used in and into the module if and only if TLS is supported.

The initial motivation for this test came during profiling of an application on an ARM Cortex A5 system (built against the Yocto project's krogoth branch but using twisted 17.1.0). I found that a large portion of the CPU load was due to the connection creator being instantiated for each to request to a third party REST API. After a brief discussion on the mailing list (see above), I submitted this ticket.

A few items I was unsure of and wanted to bring up during review:

  • It didn't seem like any new developer documentation was appropriate. While I believe the class to be useful (it is for my case), the default context for Agents is likely more appropriate for general documentation.
  • It is likely that there is a cacheSize for which a tuple would yield better performance due to better use of CPU cache (an example from C++ would be the cases where a vector outperforms a map for lookup). However, since there isn't a benchmark for HTTPS as of yet, I avoided worrying about this case on the first go-round. I felt it was better to keep the implementation simple
  • I struggled a bit with whether the _HostnameCacheEntry should use slots to be as light as possible. However, in lieu of an example where it is a problem I kept it simple.

Tested in a virtualenv on Arch Linux with and without TLS support.

comment:4 Changed 7 months ago by Jason Litzinger

Thanks for the review! I'd assumed there wasn't much interest, though I have found it useful in my setup (which really only hits one host, and thus doesn't hit the bug you found).

I'll clean this up and get it back into review.

comment:5 Changed 6 months ago by Jason Litzinger

Keywords: review added

Changes in v2:

  • Eliminate custom cache of hostnames in favor of using OrderedDict for the LRU cache.
  • Instead of re-implementing the contents of BrowserLikePolicyForHTTPS, take a policy as a parameter and cache the result of creatorForNetLoc() for use in subsequent calls with the same hostname.

One odd observation is that when I generate the apidocs locally it shows the new class as "Present since 14.0", is that due to something in my docstrings?

comment:6 Changed 4 months ago by Jason Litzinger

As it is relevant to any performance related change, I tried to work up a benchmark for HTTPS motivated by this ticket. The current code is here (I'll be opening a PR to start review soon):

This benchmark doesn't use this enhancement by default, however, when I use it on my local machine there was an improvement.


126.0 web_https/sec (630 web_https in 5 seconds)


238.0 web_https/sec (1190 web_https in 5 seconds)

Some relevant versions: cffi (1.10.0) cryptography (2.0.3) pyOpenSSL (17.2.0)

Machine Intel(R) Core(TM) i7-4500U CPU @ 1.80GHz 12G RAM 128G SSD

I've also been running this on an ARM Cortex A5 and it has made a significant improvement in load. Like any benchmark, its just a datapoint, too specific to be generally useful, but I thought I'd share.

Note: See TracTickets for help on using tickets.