Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove abandoned im crate in favor of Arc::make_mut #305

Merged
merged 5 commits into from
Jan 8, 2021
Merged

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Jan 7, 2021

This incurs a lot more cloning when using lots of scopes and breadcrumbs for example.

fixes #258

@Swatinem Swatinem requested a review from a team January 7, 2021 09:35
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arc::make_mut is a strange magical api

@Swatinem
Copy link
Member Author

Swatinem commented Jan 7, 2021

NOTE: Please don’t merge yet. There were concerns about the performance, and I would like to measure the impact first. There are some benchmarks in #306

@jan-auer jan-auer marked this pull request as draft January 7, 2021 17:00
Swatinem and others added 2 commits January 8, 2021 10:30
The benchmarks so far test the scope with tags and breadcrumbs in a few
situations:
* inactive client,
* active client, only scope manipulation
* active client, scope and capturing messages

The Benchmarks also allow measuring the "minimal API" mode, which
basically removes the complete implementation at compile time.

Measurements on my system are:

Minimal API (compile-time noop): ~2.6ns

Tags:
* inactive: 66ns
* active: 26us
* capture: 53us

Breadcrumbs:
* inactive: 2us
* active: 52us
* capture: 88us
@Swatinem
Copy link
Member Author

Swatinem commented Jan 8, 2021

Okay, I re-ran the benchmarks and get these results:

Tags:

  • inactive: 70ns +2%
  • active: 21us -18%
  • capture: 39us -26%

Breadcrumbs:

  • inactive: 2us +1%
  • active: 55us +5%
  • capture: 92us +6%

So at least with the testcase (relatively few [10-30] tags in the hashmap), we even get a nice speedup.
The breadcrumbs test (overflowing breadcrumbs even), we have ~-5%, so a small hat.

Interesting that the hashmap testcase is sped up, but it kind of makes sense, since immutable hashmaps use buckets (I think 32 elements maybe?) so with a small number of tags they don’t win us anything. I think the number of k/v pairs should be fairly small usually, but I can also create another test with a higher number.

For the breadcrumbs, the testcase runs into the default max_breadcrumbs limit and the slowdown is acceptable IMO. I guess the slowdown will get worse if people explicitly increase the number of max_breadcrumbs.

With a populated outer scope, do a few nested `with_scope` calls and
add tags / breadcrumbs in those.
Perf numbers are similar to before:
-8% for tags (speedup) and +2% for breadcrumbs, so quite negligible.
@Swatinem Swatinem marked this pull request as ready for review January 8, 2021 11:07
@Swatinem Swatinem merged commit 7745b9d into master Jan 8, 2021
@Swatinem Swatinem deleted the fix/rm-im branch January 8, 2021 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Soundness issues in dependency sized-chunks (via im crate)
2 participants