#12158 Deprecate t.w.h.HTTPClient

[twm]

Scope and purpose

Fixes #12158.

[codspeed-hq]

CodSpeed Performance Report

Merging #12171 will not alter performance

_{Comparing 12158-deprecate-httpclient (6bc3315) with trunk (5ef3fcc)}

Summary

✅ 2 untouched benchmarks

[itamarst]

The reason for the performance degradation is likely this change, this adds lots of overhead on every single attribute lookup from the module. For performance purposes, it's better to do class-level deprecation, such that user gets warning on instantiation rather than import.

[glyph]

Given that imports break when things are removed, we really need to make sure that we get warnings on imports. Class level deprecation only works if we are going to make it possible to import but impossible to instantiate. Do you think it's possible to make this sort of deprecation less performance-intensive?

[itamarst]

I tried with changing the module __class__ to a subclass of ModuleType with a property, and it had a performance hit. I don't have any other implementation ideas.

Pretty sure every other Python library in existence just deprecates instantiation, not imports, I think Twisted can live with that too. You can add a phase of "it imports but breaks when created" if you want to extend the backwards compat period but that seems silly.

[adiroiban]

just my feedback

I also think that is OK to raise the warning at __ini__.

I expect that in order to check for backard compatibility, Twisted users, will not just do an import, but rather will run their test suites agains the latest Twisted trunk.

And those tests should have at least one init.

---

A waning at import is nice... but it it comes with a performance penalty, I prefer to have better performance.

[glyph]

The performance penalty should purely be at import time though, right? Unless you're doing from x import y; ...; y.z() instead of what you're supposed to be doing, from x.y import z; ...; z()

[glyph]

If the issue is that we care about the performance of y.z() (why?) then how about this alternate suggestion:

• leave deprecatedModuleAttribute in place as it is the desired intention we want to express
• wrap the class in a warn-when-instantiated model
• add an env var that restores the slow behavior

I care a lot about the ability to actually get the advertised workflow of the compatibility policy (run your tests in CI with -Werror, get a pass, upgrade, run without -Werror, everything works) but that doesn't mean we need to care about warnings always being emitted outside of CI.

[itamarst]

Empirically we do care about the performance of y.z(), my experience was that removed a deprecatedModule in http.py did make a difference. This is something that could be fixed by refactoring the code to instead do "from y import z". E.g. if you look in twisted/web/server.py there's a bunch of calls to http.Request.write().

So if we switched to "from twisted.web.http import Request" and other similar changes, that would make the performance impacts much less relevant. Would still impact anyone third party who is doing e.g. "http.CREATED", or whatever. That's probably less frequent than the calls in server.py so might care about it less.

[glyph]

Maybe we could do both then. Start by changing the implementation of deprecatedModuleAttribute to only do the slow thing with the env var, replace some calls with proper relative imports (which will probably be a marginal perf increase in its own right, since that's just faster anyway) then put back the slow default behavior if it's not actually impacting the benchmarks sufficiently after that change?

[itamarst]

I did the second part. Haven't done the first part, would you like to? 😁

[glyph]

I did the second part. Haven't done the first part, would you like to? 😁

haha, hoist by my own petard