[BUG]: DD_PROFILING_MEMORY_ENABLED=true slows down application significantly #13307
Comments
|
@bmalehorn thanks again for trying different configurations with the latest ddtrace version. It helped us to narrow down the root cause. The heap allocation profiler keeps a list of samples it has seen, and it does O(samples) scan for every free. Probably this is why it's fast for first few batches and it's much slower on the later ones. We will prioritize fixing this issue. |
The heap profiler component of memalloc uses a plain array to track allocations. It performs a linear scan of this array on every free to see if the freed allocation is tracked. This means the cost of freeing an allocation increases substantially for programs with large heaps. Switch to an unordered map for tracking allocations. This should have roughly constant overhead even for larger heaps, where we track more allocations. Rather than find or implement a map in C, use C++. We can wrap a std::unordered_map in a thin compatibility layer and use it from the rest of memalloc without having to completely refactor memalloc. Making this work requires some awkward modifications to setup.py to build memalloc. Basically, the Extension class can have sources from multiple languages, but it isn't possible to pass different flags per language, such as the language standard. Instead, we can use a method borrowed from a Stack Overflow answer and use the build_clib command, extended to support compiler arguments. This lets us build a static library, which we can link from memalloc. Note that with the way we build the library, it'll be passed to the link step for other C extensions as well. This should be harmless, though. The other extensions will only include symbols they actually use, which in practice will be none. We'll only have trouble if there are duplicated definitions, but the functions in the map library here are all "namespaced". Note also that we might get slightly worse performance due to the extra cost of map access for very small heaps, compared to using a simple array. TODO - demonstrate the difference in performance with this change for large and small heaps. Fixes #13307
|
No problem! To confirm more and build confidence to re-enable once fixed, I profiled the C stack frames by running a command like this: With memory profiling turned on:
I see 77% of time was spent in And with memory profiling turned off: 
Versions used in this test: |
|
Hey @nsrip-dd , thanks for the links. I tried testing this out: RUN poetry install
+RUN poetry run pip install /common/ddtrace-3.7.0.dev47+g5eaa19d9c-cp310-cp310-manylinux_2_17_aarch64.manylinux2014_aarch64.whlin the repro: It does run faster, but now profiles do not show any heap live size information: is that expected? I also tried running with I also checked profiles I got from running on 3.6.0 today. They did contain heap live size: |
|
@bmalehorn Thanks a bunch for trying it out! That is definitely not expected. I suspect that you might be running into issues linking the C++ library at run time (the memalloc import currently fails silently). To confirm, could try Edit: I've tried out using a C hash table instead to get rid of the C++ dependency, so if that is the issue you ran into, and you're up for trying another version, this build should work better: wheels-cp310-manylinux_aarch64. |
|
@nsrip-dd Ah yep, it was an import error: I tried installing the new wheel: Looks like the import is OK. Running the reproduction, now we do see memory profiles: 
And it did indeed run faster! total time to load 1009015 rows: 92 seconds I captured a profile during that time: Only 0.7% of time was spent in
No rush on our end, we probably are not going to upgrade ddtrace immediately. We're mostly just happy to have identified the issue & have a fix in progress that we'll pick up when we upgrade ddtrace. |
|
On a related note about the upgrades, will this bug fix be ported back to ddtrace 2? |
Yeah, we'll backport this to 2.21 |
…13317) The heap profiler component of memalloc uses a plain array to track allocations. It performs a linear scan of this array on every free to see if the freed allocation is tracked. This means the cost of freeing an allocation increases substantially for programs with large heaps. Switch to a hash table for tracking allocations. This should have roughly constant overhead even for larger heaps, where we track more allocations. This PR uses the C implementation of the [Abseil SwissTable](https://abseil.io/blog/20180927-swisstables) from https://github.com/google/cwisstable. This hash table is designed for memory efficiency, both in terms of memory used (1 byte per entry of metadata) and in terms of access (cache-friendly layout). The table is designed in particular for fast lookup and insertion. We do a lookup for every single free, so it's important that it's fast. This PR previously tried to use a C++ `std::unordered_map`, but it proved far too difficult to integrate with the existing C `memalloc` implementation. This PR includes the single `cwisstable.h` header with the following modifications, which can be found in the header by searching for `BEGIN MODIFICATION`: - Fix `CWISS_Mul128`: for 64-bit windows we need a different intrinsic, and for 32-bit systems we can't use it at all - Fix a multiple declaration and invalid C++ syntax in `CWISS_LeadingZeroes64` for 32-bit Windows The PR also supplies a 32-bit compatible hash function since the one in `cwisstable.h` couldn't be easily made to work. We use `xxHash` instead. Note that `cwisstable.h` requires the hash function to return a `size_t`, which is 32 bits on 32-bit platforms, but the SwissTable design expects 64-bit hashes. I don't know if this is a huge problem in practice or if people even use our profiler on 32-bit systems, but it's something to watch out for. Note that the `cwisstable.h` hash table implementation basically never gets rid of backing memory unless we completely clear the table. This is not a big deal in practice. Each entry in the table is an 8 byte `void*` key and an 8 byte `traceback_t*`, plus 1 byte of control metadata. If we assume a target load factor of 50%, and we keep the existing `2^16` element cap, then a completely full table is only about 2MiB. Most of the heap profiler memory usage comes from the tracebacks themselves, which we _do_ free when they're removed from the profiler structures. This PR also fixes a potential memory leak. Since we use try-locks, it theoretically possible to fail to un-track an allocation if we can't acquire the lock. When this happens, the previous implementation would just leak an entry. With the hash table, if we find an existing entry for a given address, we can free the old traceback and replace it with the new one. The real fix will be to fix how we handle locking, but this PR improves the situation. I've tested this locally and see very good performance even for larger heaps, and this has also been tested in #13307. Fixes #13307
…backport 2.21] (#13317) The heap profiler component of memalloc uses a plain array to track allocations. It performs a linear scan of this array on every free to see if the freed allocation is tracked. This means the cost of freeing an allocation increases substantially for programs with large heaps. Switch to a hash table for tracking allocations. This should have roughly constant overhead even for larger heaps, where we track more allocations. This PR uses the C implementation of the [Abseil SwissTable](https://abseil.io/blog/20180927-swisstables) from https://github.com/google/cwisstable. This hash table is designed for memory efficiency, both in terms of memory used (1 byte per entry of metadata) and in terms of access (cache-friendly layout). The table is designed in particular for fast lookup and insertion. We do a lookup for every single free, so it's important that it's fast. This PR previously tried to use a C++ `std::unordered_map`, but it proved far too difficult to integrate with the existing C `memalloc` implementation. This PR includes the single `cwisstable.h` header with the following modifications, which can be found in the header by searching for `BEGIN MODIFICATION`: - Fix `CWISS_Mul128`: for 64-bit windows we need a different intrinsic, and for 32-bit systems we can't use it at all - Fix a multiple declaration and invalid C++ syntax in `CWISS_LeadingZeroes64` for 32-bit Windows The PR also supplies a 32-bit compatible hash function since the one in `cwisstable.h` couldn't be easily made to work. We use `xxHash` instead. Note that `cwisstable.h` requires the hash function to return a `size_t`, which is 32 bits on 32-bit platforms, but the SwissTable design expects 64-bit hashes. I don't know if this is a huge problem in practice or if people even use our profiler on 32-bit systems, but it's something to watch out for. Note that the `cwisstable.h` hash table implementation basically never gets rid of backing memory unless we completely clear the table. This is not a big deal in practice. Each entry in the table is an 8 byte `void*` key and an 8 byte `traceback_t*`, plus 1 byte of control metadata. If we assume a target load factor of 50%, and we keep the existing `2^16` element cap, then a completely full table is only about 2MiB. Most of the heap profiler memory usage comes from the tracebacks themselves, which we _do_ free when they're removed from the profiler structures. This PR also fixes a potential memory leak. Since we use try-locks, it theoretically possible to fail to un-track an allocation if we can't acquire the lock. When this happens, the previous implementation would just leak an entry. With the hash table, if we find an existing entry for a given address, we can free the old traceback and replace it with the new one. The real fix will be to fix how we handle locking, but this PR improves the situation. I've tested this locally and see very good performance even for larger heaps, and this has also been tested in #13307. Fixes #13307
…backport 2.21] (#13317) (#13350) Backports #13317 to 2.21, including the fix from #13353 The heap profiler component of memalloc uses a plain array to track allocations. It performs a linear scan of this array on every free to see if the freed allocation is tracked. This means the cost of freeing an allocation increases substantially for programs with large heaps. Switch to a hash table for tracking allocations. This should have roughly constant overhead even for larger heaps, where we track more allocations. This PR uses the C implementation of the [Abseil SwissTable](https://abseil.io/blog/20180927-swisstables) from https://github.com/google/cwisstable. This hash table is designed for memory efficiency, both in terms of memory used (1 byte per entry of metadata) and in terms of access (cache-friendly layout). The table is designed in particular for fast lookup and insertion. We do a lookup for every single free, so it's important that it's fast. This PR previously tried to use a C++ `std::unordered_map`, but it proved far too difficult to integrate with the existing C `memalloc` implementation. This PR includes the single `cwisstable.h` header with the following modifications, which can be found in the header by searching for `BEGIN MODIFICATION`: - Fix `CWISS_Mul128`: for 64-bit windows we need a different intrinsic, and for 32-bit systems we can't use it at all - Fix a multiple declaration and invalid C++ syntax in `CWISS_LeadingZeroes64` for 32-bit Windows The PR also supplies a 32-bit compatible hash function since the one in `cwisstable.h` couldn't be easily made to work. We use `xxHash` instead. Note that `cwisstable.h` requires the hash function to return a `size_t`, which is 32 bits on 32-bit platforms, but the SwissTable design expects 64-bit hashes. I don't know if this is a huge problem in practice or if people even use our profiler on 32-bit systems, but it's something to watch out for. Note that the `cwisstable.h` hash table implementation basically never gets rid of backing memory unless we completely clear the table. This is not a big deal in practice. Each entry in the table is an 8 byte `void*` key and an 8 byte `traceback_t*`, plus 1 byte of control metadata. If we assume a target load factor of 50%, and we keep the existing `2^16` element cap, then a completely full table is only about 2MiB. Most of the heap profiler memory usage comes from the tracebacks themselves, which we _do_ free when they're removed from the profiler structures. This PR also fixes a potential memory leak. Since we use try-locks, it theoretically possible to fail to un-track an allocation if we can't acquire the lock. When this happens, the previous implementation would just leak an entry. With the hash table, if we find an existing entry for a given address, we can free the old traceback and replace it with the new one. The real fix will be to fix how we handle locking, but this PR improves the situation. I've tested this locally and see very good performance even for larger heaps, and this has also been tested in #13307. Fixes #13307 ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
Uh oh!
There was an error while loading. Please reload this page.
Tracer Version(s)
ddtrace 3.6.0
Python Version(s)
Python 3.10.6
Pip Version(s)
pip 22.3.1
Bug Report
Hello,
We've noticed a significant slowdown in one of our applications in production that seems to be related to memory size.
This application pulls in quite a lot of data, and stores it all in memory. Specifically it fetches about 1 million rows of a table, in batches of 10k at a time, and stores them as ORM objects in a big list. Additionally the slowdown increases over time, with the queries starting out fairly fast, but each query gets slower and slower over time.
I reproduced this in my local environment and ultimately made a 5x speedup by disabling the memory profiler via DD_PROFILING_MEMORY_ENABLED=false - see below for some evidence.
Discussed on the public Datadog Slack here: https://datadoghq.slack.com/archives/C0633E2KH16/p1746038872275079 and in a private message with @taegyunkim.
Org id: 550746 (Lumos)
Reproduction Code
I have a reproduction in my local environment. With memory profiling turned off:
I log how long each batch takes, and each batch runs fairly quickly:
total time to load 1009015 rows: 108 seconds
When I run with memory profiling enabled:
Running the job, it starts out fast:
but after running for several minutes, it's quite slow, and getting slower over time:
total time to load 1009015 rows: 507 seconds
Error Logs
Here are some relevant links for myself & others:
brian/profiler-perf-all-ddtrace-3.6.0Libraries in Use
Relevant libraries used in this reproduction:
Operating System
Linux 8ea13d816b46 6.10.14-linuxkit #1 SMP Fri Nov 29 17:22:03 UTC 2024 aarch64 GNU/Linux
The text was updated successfully, but these errors were encountered: