Speed up Local

[akx]

While profiling a Django app with viztracer, I noticed asgiref Local access/locking being actually visible in the trace, and figured there could be something to fix perf-wise :)

@contextlib.contextmanager can be slow due to the fact that the interpreter needs to juggle a generator frame, etc. This PR replaces it with a small real context manager (that is stored on the instance for reuse instead, in the first commit.

The second commit further specializes Local (using __new__ magic, so this is transparent to consumers) to 2 classes depending on if thread_critical is set, so accesses do not require branches at all – and indeed, in the thread-critical case, nullcontext isn't required anymore. (Further, the guards in the setattr case become unnecessary since we can just use object.__setattr__ to sidestep the classes' impls.)

EDIT: the third commit gets rid of the context manager altogether in favor of just inlining the same things as a try:finally: block, as per comments here, for even more performance. 😄

I pytest-benchmarked this to be pleasantly faster (and given that these are often very hot paths in Django, this is a nice win for not an awful lot of additional code):

Real context manager

Benchmark	Kops/s before	Kops/s after	Change
test_getattr_thread_critical	1,041.50	3,715.52	+257%
test_setattr_thread_critical	1,040.11	3,575.43	+244%
test_getattr_default	1,034.84	2,493.43	+141%
test_setattr_default	913.03	1,833.74	+101%
test_delattr_thread_critical	504.15	1,747.49	+247%
test_getattr_missing_default	544.32	968.90	+78%
test_delattr_default	444.68	1,030.41	+132%
test_getattr_thread_critical_async	31.40	32.50	+4%
test_setattr_thread_critical_async	32.69	33.34	+2%

No context manager (only affects default)

Benchmark	Kops/s before	Kops/s after	Change
test_getattr_thread_critical	1,047.4632	3,649.6306	+248.4%
test_setattr_thread_critical	1,013.8543	3,500.2482	+245.2%
test_getattr_default	986.0904	3,586.0984	+263.7%
test_setattr_default	859.1037	2,673.6265	+211.2%
test_getattr_missing_default	520.0822	1,068.1260	+105.4%
test_delattr_thread_critical	502.1324	1,712.0688	+241.0%
test_delattr_default	423.2087	1,347.7917	+218.5%
test_setattr_thread_critical_async	31.4873	32.1533	+2.1%
test_getattr_thread_critical_async	30.7843	31.7018	+3.0%

[bluetech]

I have seen Local show up in profiles as well, and I don't even use async, so it'd be great to have its performance improved.

I wonder given that Local is performance sensitive if it won't be better to just use try/finally for the lock instead of the _LockContext helper? Sure it's not as nice and a bit more error prone, but I think this is a case where efficiency beats beauty (of course as long as avoiding _LockContext is actually faster).

[akx]

I wonder given that Local is performance sensitive if it won't be better to just use try/finally for the lock instead of the _LockContext helper?

Sure. Triplicating the logic into the functions would probably make a lot of sense for performance, but those functions should probably be comment-marked "there be dragons here, be very careful" 😄

[akx]

@bluetech Great news: according to my measurements it is indeed a whole lot faster, another +100% on top of the context-manager version 😄

Name (time in ns)                      0001_baselin(*) OPS (Kops/s)  0002_real-co OPS (Kops/s)  ΔOPS (Kops/s)  0003_real-co OPS (Kops/s)  ΔOPS (Kops/s)
-------------------------------------------------------------------------------------------------------------------------------------------------------
test_getattr_default                                       986.0904                 2,318.5320        +135.1%                 3,586.0984        +263.7%
test_setattr_default                                       859.1037                 1,839.8386        +114.2%                 2,673.6265        +211.2%
test_getattr_missing_default                               520.0822                   941.0258         +80.9%                 1,068.1260        +105.4%
test_delattr_default                                       423.2087                   988.1233        +133.5%                 1,347.7917        +218.5%

[akx]

Gentle review nudge, @carltongibson? 😄 (Ditto for #552)

[carltongibson]

@akx Nice 😃 — I'm just working my way back to asgiref now, as it happens. 🎁

[carltongibson]

@akx I'm going to defer this one for now. I've been focused on a whole load of deadlock issues, and want to get the fixes for those out before taking on thinking about this properly.

[akx]

@carltongibson Okay 👍 I rebased this in any case. OPS gains remain approximately the same as before:

Name (time in ns)                      0001_deda0d4 OPS (Kops/s)  0002_13dc288 OPS (Kops/s)  ΔOPS (Kops/s)
----------------------------------------------------------------------------------------------------------
test_getattr_thread_critical                          1,058.0157                 3,674.1259        +247.3%
test_setattr_thread_critical                            996.5607                 3,545.8994        +255.8%
test_getattr_default                                    951.1677                 3,314.3766        +248.5%
test_setattr_default                                    740.9254                 1,708.1379        +130.5%
test_getattr_missing_default                            491.4737                   914.1192         +86.0%
test_delattr_thread_critical                            504.7236                 1,733.1487        +243.4%
test_delattr_default                                    375.2765                   864.4175        +130.3%
test_getattr_thread_critical_async                       30.9839                    33.5124          +8.2%
test_setattr_thread_critical_async                       32.2793                    32.7901          +1.6%

[carltongibson]

Super. The rebase is helpful

[akx]

Rebased. Still benchmarks pleasantly faster against 3.12.1 (a7cc976) 😄

by min time

-------------------------- benchmark: 9 tests, 2 sources --------------------------
Name (time in ns)                      0001_a7cc976 Min  0002_6030d30 Min      ΔMin
-----------------------------------------------------------------------------------
test_getattr_thread_critical                   874.9776          208.1506    -76.2%
test_setattr_thread_critical                   974.9900          270.8338    -72.2%
test_getattr_default                         1,008.2033          270.8111    -73.1%
test_setattr_default                         1,250.0677          499.8874    -60.0%
test_getattr_missing_default                 1,874.9852          959.0294    -48.9%
test_delattr_thread_critical                 1,916.6619          522.9493    -72.7%
test_delattr_default                         2,540.8808        1,040.9858    -59.0%
test_setattr_thread_critical_async          20,874.8970       19,249.9720     -7.8%
test_getattr_thread_critical_async          23,166.8819       20,874.8970     -9.9%
-----------------------------------------------------------------------------------

by average ops

------------------------------------- benchmark: 9 tests, 2 sources --------------------------------------
Name (time in ns)                      0001_a7cc976 OPS (Kops/s)  0002_6030d30 OPS (Kops/s)  ΔOPS (Kops/s)
----------------------------------------------------------------------------------------------------------
test_getattr_thread_critical                            984.1940                 3,070.1211        +211.9%
test_setattr_thread_critical                            967.3637                 3,507.0525        +262.5%
test_getattr_default                                    924.4579                 3,281.1620        +254.9%
test_setattr_default                                    697.0743                 1,631.3097        +134.0%
test_getattr_missing_default                            476.1885                   887.4339         +86.4%
test_delattr_thread_critical                            492.5381                 1,727.1412        +250.7%
test_delattr_default                                    352.3465                   843.5477        +139.4%
test_setattr_thread_critical_async                       30.9745                    33.0855          +6.8%
test_getattr_thread_critical_async                       29.3506                    32.3154         +10.1%
----------------------------------------------------------------------------------------------------------

[carltongibson]

Hey @akx. Right, I finally got to sit down with this with a clear head.

So, yes. OK I see you.

My concern is maintenance, especially with the repetitive code. So, comments and, even better, tests. How do we stop people (me) editing this wrong in the future, and how do we tell if they (I) have?

[akx]

My concern is maintenance, especially with the repetitive code.

So, comments and

There were, IMO, fairly copious comments (most of the critical, tricksy ones being from the original source; see the diff without whitespace changes to make that more clear) but now there's a couple more. 😄

, even better, tests.

I added the benchmark test file I used (took some finagling to make it pass mypy). It checks whether a suitable benchmark plugin is installed (either pytest-benchmark or pytest-codspeed).

I also added a couple of lines to test_local.py so the coverage, according to pytest --cov, with pytest-cov installed) for that file is now a solid 100%. (I don't see coverage being measured in GitHub Actions CI; should it?)

How do we stop people (me) editing this wrong in the future, and how do we tell if they (I) have?

I would hope test_local.py is enough to catch correctness bugs in this module, but I think we can add more belt-and-suspenders tests if you like?

As for the other wrongness, speed regressions: You could run the aforementioned benchmark for this module locally on the preimage and edited version, and see if something broke (more than ±10%; that seems to be the usual measurement noise margin).

For a more reproducible and non-local approach: I've been working on speeding up Pillow, and codspeed was set up there (python-pillow/Pillow#9654) to run benchmarks during CI too.

[akx]

@carltongibson Good instinct though - I did find a small pre-existing corner case bug that's separately fixed in #576 and this PR is now stacked on top of that commit.