Make BoundedAttributes constructor more efficient #3331

torarvid · 2023-06-02T13:18:48Z

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.

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:
No.

Checklist:

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

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.

linux-foundation-easycla · 2023-06-02T13:18:51Z

The committers listed above are authorized under a signed CLA.

✅ login: torarvid / name: Tor Arvid Lund (eaaa965, efa046e)
✅ login: srikanthccv / name: Srikanth Chekuri (021787e)
✅ login: pmcollins / name: Pablo Collins (4a927e1)
✅ login: ocelotl / name: Diego Hurtado (6128e00)

pmcollins · 2023-06-05T20:05:57Z

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 · 2023-06-22T02:32:40Z

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 · 2023-07-05T09:10:07Z

opentelemetry-api/src/opentelemetry/attributes/__init__.py

@@ -162,25 +162,41 @@ def __repr__(self):
    def __getitem__(self, key):
        return self._dict[key]

+    def _unsafe__setitem(self, key, value):


Maybe use unlocked instead of unsafe? It is more accurate IMO.

Or maybe thread_unsafe?

ocelotl · 2023-07-05T09:31:00Z

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 · 2023-11-28T18:55:55Z

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

torarvid · 2023-11-28T19:27:03Z

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 · 2023-11-29T18:56:44Z

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
--------------------------------------------------------------------------------------------------------------------------------------------------

# Conflicts: # opentelemetry-api/src/opentelemetry/attributes/__init__.py

pmcollins · 2024-03-18T20:40:36Z

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 · 2024-03-20T20:59:22Z

Shall we close this ticket?

ocelotl · 2024-06-28T23:07:21Z

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

ocelotl · 2024-07-11T16:21:23Z

Discussed in the SIG, agreed to close this PR.

torarvid requested a review from a team as a code owner June 2, 2023 13:18

torarvid and others added 2 commits June 4, 2023 19:47

black

eaaa965

Merge branch 'main' into faster-boundedattributes-ctor

021787e

Merge branch 'main' into faster-boundedattributes-ctor

6128e00

ocelotl reviewed Jul 5, 2023

View reviewed changes

pmcollins mentioned this pull request Nov 15, 2023

Add tests to boundedattributes ctor PR #3531

Closed

pmcollins force-pushed the faster-boundedattributes-ctor branch from 238b8e8 to 6128e00 Compare March 12, 2024 19:43

Merge branch 'main' into faster-boundedattributes-ctor

4a927e1

# Conflicts: # opentelemetry-api/src/opentelemetry/attributes/__init__.py

ocelotl closed this Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make BoundedAttributes constructor more efficient #3331

Make BoundedAttributes constructor more efficient #3331

torarvid commented Jun 2, 2023

linux-foundation-easycla bot commented Jun 2, 2023 •

edited

Loading

pmcollins commented Jun 5, 2023

aabmass commented Jun 22, 2023

ocelotl Jul 5, 2023

ocelotl Jul 5, 2023

ocelotl commented Jul 5, 2023

pmcollins commented Nov 28, 2023

torarvid commented Nov 28, 2023

pmcollins commented Nov 29, 2023

pmcollins commented Mar 18, 2024

pmcollins commented Mar 20, 2024

ocelotl commented Jun 28, 2024

ocelotl commented Jul 11, 2024

Make BoundedAttributes constructor more efficient #3331

Make BoundedAttributes constructor more efficient #3331

Conversation

torarvid commented Jun 2, 2023

Description

Motivation

Type of change

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

linux-foundation-easycla bot commented Jun 2, 2023 • edited Loading

pmcollins commented Jun 5, 2023

aabmass commented Jun 22, 2023

ocelotl Jul 5, 2023

Choose a reason for hiding this comment

ocelotl Jul 5, 2023

Choose a reason for hiding this comment

ocelotl commented Jul 5, 2023

pmcollins commented Nov 28, 2023

torarvid commented Nov 28, 2023

pmcollins commented Nov 29, 2023

pmcollins commented Mar 18, 2024

pmcollins commented Mar 20, 2024

ocelotl commented Jun 28, 2024

ocelotl commented Jul 11, 2024

linux-foundation-easycla bot commented Jun 2, 2023 •

edited

Loading