CodSpeed Performance Report

Merging #12446 will degrade performances by 98.96%

_{Comparing allrob23:patch-fix-test-publisher-bench (7362817) with trunk (aedee3e)}

Summary

❌ 1 regressions
✅ 35 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

please review

@allrob23 It looks like the only issue here is that you forgot to file an issue describing the problem first and adding a news fragment. The fragment itself could be a .misc, so hopefully this is just a few minutes of paperwork to get things unstuck?

@glyph maybe in some cases we can use the PR id instead of the issue id... to reduce the red tape

I understand that we have an issue, and that issue might not be fixed with the first proposed PR...

but if this PR is not merged, the newsfragment is also not merged

Just trying to make it easier for people to contribute

maybe in some cases we can use the PR id instead of the issue id... to reduce the red tape

I think that writing down the issue that you plan to fix before writing any code is a really good habit that I wish felt less like "red tape". But, given that we seem to be going against the grain, perhaps in the case where we would accept a .misc anyway, we can accept a .misc with the PR number? (I do think that we really need to insist on the red tape of "I explicitly considered whether this belongs in the changelog or not", even if we don't need an issue)

"I explicitly considered whether this belongs in the changelog or not", even if we don't need an issue)

I fully agree with creating a news fragment file for every PR.

The question is about creating a GitHub Issue, when you already have a Github PR with a description of the "problem"

Hello @adiroiban and @glyph , surry but after your conversation I don't quite understand what I need to do, do I need to create the Issue? Is there something else I should do?

do I need to create the Issue? Is there something else I should do?

Adi already created the news fragment for you, so you're all set here as far as that goes. I guess the sticking point right now is that I don't understand the massive benchmark regression here. wouldn't emptying out __call__ make the benchmark faster? If you're expecting it to get 100% slower, then I guess we can move forward :)

Additionally, the benchmark test a test was not adding observers in the go function, missing a part of the setup that affects the performance measurement.

The setup is excluded from the benchmark measurement on purpose. The benchmark that captures addObserver is benchmarks/test_log_publisher.py. This benchmark is testing log dispatch.

It's possible that we might want to replace DummyObserver with lambda event: None or something even lower-overhead than a single attribute set, but as it stands, this change would basically break the benchmarks, so I am going to close this PR.