Make BoundedAttributes constructor more efficient

[torarvid]

trafficstars

Description

There is no risk of locking issues during a constructor call. It is also not desirable to lock-unlock-... for each provided attribute to the constructor.

This commit seeks to improve on these by refactoring __setitem__ such that the 'body' inside the lock context manager is moved to a separate method _unsafe__setitem. This method can then be called by internal methods without taking the lock first.

Another method update (overridden from MutableMapping) is added that takes the lock once and then calls another new method _unsafe_update, which in turn simply iterates the attribute items and calls _unsafe__setitem on them.

(I haven't created a corresponding issue, please tell me if this is required)

Motivation

We use opentelemetry-python at work, and this constructor is showing up quite a bit in our cpu profiles when using py-spy.

Type of change

Please delete options that are not relevant.

• [x] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I have simply run tox locally 😬 🙈

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

• [ ] Yes. - Link to PR:
• [x] No.

Checklist:

• [x] Followed the style guidelines of this project (I think so)
• [ ] Changelogs have been updated
• [ ] Unit tests have been added
• [ ] Documentation has been updated

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: torarvid / name: Tor Arvid Lund (eaaa9651e5bf1d34d16686b00f494b181171fd8b, efa046ef4fd3ea644d95e6a182c0f8b6c43c382c)
• :white_check_mark: login: srikanthccv / name: Srikanth Chekuri (021787eeecaa92783a3235cd6b279c38752d6e18)
• :white_check_mark: login: pmcollins / name: Pablo Collins (4a927e18a172db0b1b40efc97ae28b80dcae3b72)
• :white_check_mark: login: ocelotl / name: Diego Hurtado (6128e00247e640aaf6e64e7fc72f59907b57422d)

[pmcollins]

These changes seem fine to me. I'm still ramping up on this repo so hopefully others will correct me if I'm overstepping, but what do you think about adding test coverage for correctness?

[aabmass]

Thanks for the PR!

We use opentelemetry-python at work, and this constructor is showing up quite a bit in our cpu profiles when using py-spy.

I would expect this to be a hot function if you're sampling a lot because it is called every span creation (code), but it's not clear how much the PR actual improves performance.

I am also on board with something like this, but it would be nice to see at least some timeit results on improvement here. We could also add benchmark tests similar to these.

Did you see specifically that the locking was taking the most time in the profile?

[ocelotl]

These changes seem fine to me. I'm still ramping up on this repo so hopefully others will correct me if I'm overstepping, but what do you think about adding test coverage for correctness?

Right, we need at least unit tests for these changes.

[pmcollins]

Hi @torarvid, thank you for your help. Do you have availability to continue with this PR?

[torarvid]

Hi @torarvid, thank you for your help. Do you have availability to continue with this PR?

Hi, @pmcollins 👋 Sorry for the huge delay here. I'm currently on paternity leave (and have been for a few months), so very little time for coding at the moment.

I have seen the request for some timeit results, and I believe it's possible I have jumped the gun with this PR... 🤦‍♂️ I realize now that I of course should have tested the performance of this before claiming the locking was the issue — but I didn't do that before creating the patch. So it's entirely possible that I'm not really improving things here.

If there is anyone who wants to take this over, anyone should feel free to do that. Otherwise, I'm still interested in improving things and will be back at work start of January.

[pmcollins]

Thanks torarvid. We're happy to take it from here.

Based on some simple testing, this change appears to yield a small (~2%) performance improvement in constructor execution time.

Will discuss with the SIG how proceed with this and other pending PRs as well. Thanks!

def test_bounded_attributes_performance(benchmark):
    def bounded_attributes_constructor_performance():
        BoundedAttributes(attributes={})

    benchmark(bounded_attributes_constructor_performance)

Before:

--------------------------------------------------------------- benchmark: 1 tests ---------------------------------------------------------------
Name (time in ns)                            Min          Max      Mean      StdDev    Median      IQR  Outliers  OPS (Mops/s)  Rounds  Iterations
--------------------------------------------------------------------------------------------------------------------------------------------------
test_bounded_attributes_performance     614.6729  57,482.1606  738.1026  1,209.0810  659.8420  35.8559  431;2713        1.3548  108426           1
--------------------------------------------------------------------------------------------------------------------------------------------------

After:

--------------------------------------------------------------- benchmark: 1 tests ---------------------------------------------------------------
Name (time in ns)                            Min          Max      Mean      StdDev    Median      IQR  Outliers  OPS (Mops/s)  Rounds  Iterations
--------------------------------------------------------------------------------------------------------------------------------------------------
test_bounded_attributes_performance     611.8789  71,141.8688  721.6176  1,209.3374  652.3915  25.6114  313;2468        1.3858   89895           1
--------------------------------------------------------------------------------------------------------------------------------------------------

[pmcollins]

My thought after looking at this PR again is that the 2% performance improvement in the constructor may not be worth the added complexity at this point (it really depends on how often this constructor runs :). If we do want to pursue this, I think a simpler implementation would be to temporarily use a no-op lock during constructor execution.

[pmcollins]

Shall we close this ticket?

[ocelotl]

I think we can close this PR and reopen when it has test cases.

[ocelotl]

Discussed in the SIG, agreed to close this PR.