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

Stop wasting time on malloc in snprintf_zstd_header #15721

Merged
merged 1 commit into from Jan 12, 2024

Conversation

rincebrain
Copy link
Contributor

Profiling zdb -vvvvv on datasets with a lot of zstd blocks, we find ourselves spending quite a lot of time on malloc/free, because we allocate a 16M abd each call, and never free it, so we're leaking 16M per call as well.

This seems sub-optimal. So let's just keep the buffer around and reuse it.

Motivation and Context

Leaking 16M per zstd block seems bad, actually.

Description

It's a static allocation. That's really it.

How Has This Been Tested?

It ran and I was no longer spending all my time in malloc/free/clear_page_erms.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Profiling zdb -vvvvv on datasets with a lot of zstd blocks, we find
ourselves spending quite a lot of time on malloc/free, because we
allocate a 16M abd each call, and never free it, so we're leaking
16M per call as well.

This seems sub-optimal. So let's just keep the buffer around and
reuse it.

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

Sure leaks per block are bad, but why do we need a leek at all? Couldn't we allocate just as much as we need instead of 16MB and then free?

@rincebrain
Copy link
Contributor Author

rincebrain commented Dec 30, 2023

The problem is that if you allocate and free each loop, then you spend most of your time in malloc/free/the bzero equivalent.

So yes, I could punt the allocation up a level and make all the callers hand in an abd to use, and refactor them all to not allocate naively, but it's not really clear to me why that's a better fit here.

Copy link
Contributor

@robn robn left a comment

Choose a reason for hiding this comment

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

I don't love it, but it does the job and is obviously better than before.

I did enough code read to satisfy myself that there's only gonna be one thread calling this.

@rincebrain
Copy link
Contributor Author

If nothing else, I'm pretty sure the print output would get horrendously mangled if it were called in parallel without explicit locking.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jan 12, 2024
@behlendorf behlendorf merged commit 6138af8 into openzfs:master Jan 12, 2024
23 of 26 checks passed
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jan 29, 2024
Profiling zdb -vvvvv on datasets with a lot of zstd blocks, we find
ourselves spending quite a lot of time on malloc/free, because we
allocate a 16M abd each call, and never free it, so we're leaking
16M per call as well.

This seems sub-optimal. So let's just keep the buffer around and
reuse it.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Rob Norris <robn@despairlabs.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#15721
behlendorf pushed a commit that referenced this pull request Jan 29, 2024
Profiling zdb -vvvvv on datasets with a lot of zstd blocks, we find
ourselves spending quite a lot of time on malloc/free, because we
allocate a 16M abd each call, and never free it, so we're leaking
16M per call as well.

This seems sub-optimal. So let's just keep the buffer around and
reuse it.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Rob Norris <robn@despairlabs.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes #15721
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
Profiling zdb -vvvvv on datasets with a lot of zstd blocks, we find
ourselves spending quite a lot of time on malloc/free, because we
allocate a 16M abd each call, and never free it, so we're leaking
16M per call as well.

This seems sub-optimal. So let's just keep the buffer around and
reuse it.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Rob Norris <robn@despairlabs.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#15721
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
Profiling zdb -vvvvv on datasets with a lot of zstd blocks, we find
ourselves spending quite a lot of time on malloc/free, because we
allocate a 16M abd each call, and never free it, so we're leaking
16M per call as well.

This seems sub-optimal. So let's just keep the buffer around and
reuse it.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Rob Norris <robn@despairlabs.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#15721
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants