Warn on memory allocation #50
Conversation
…located more frequently
| bool m_allocate_on_device = false; | ||
| // Callback executed when a segment is allocated. Can be used to track the number of actual | ||
| // memory allocations. | ||
| const segment_alloc_cb_type& m_segment_alloc_cb; |
There was a problem hiding this comment.
Please correct me if I'm missing something: I think this will be a dangling reference when using the default value in the fixed_size_heap constructor. The nullptr will produce a prvalue for the call to the constructor. The const& in the constructor will keep the prvalue alive until the call to the constructor finishes. The const& member will point to the prvalue that has been destroyed once the constructor call finishes.
Options:
- make the default point to a static variable
- pass and store the callback by value
The second option is seemingly simple, but the reference to the parent class is hidden in the captured this. Safe in the default case, but may be too implicit in the non-default case?
| (s_large_limit << (i + 1)), m_never_free, m_num_reserve_segments); | ||
| if (m_config.m_num_huge_alloc_warn_threshold > 0) | ||
| m_huge_segment_alloc_cb = [this] () { | ||
| m_num_huge_alloc += 1; |
There was a problem hiding this comment.
Open question that we didn't find a final answer for: Does this need to be atomic since this may be incremented concurrently for different heaps?
I think our conclusion was that each pool is thread safe, but e.g. updating a map of pools is not under a lock, so the assumption is that at this level allocations are anyway serialized? Thus no need for an atomic?
| detail::get_env_size_t("HWMALLOC_TINY_SEGMENT_SIZE", (1u << 16)), // 64KiB | ||
| detail::get_env_size_t("HWMALLOC_SMALL_SEGMENT_SIZE", (1u << 16)), // 64KiB | ||
| detail::get_env_size_t("HWMALLOC_LARGE_SEGMENT_SIZE", (1u << 21)), // 2MiB | ||
| detail::get_env_size_t("HWMALLOC_NUM_HUGE_ALLOC_WARN_THERESHOLD", 10) |
There was a problem hiding this comment.
| detail::get_env_size_t("HWMALLOC_NUM_HUGE_ALLOC_WARN_THERESHOLD", 10) | |
| detail::get_env_size_t("HWMALLOC_NUM_HUGE_ALLOC_WARN_THRESHOLD", 10) |
| if (!m_num_huge_alloc_did_warn && m_num_huge_alloc >= m_config.m_num_huge_alloc_warn_threshold) { | ||
| m_num_huge_alloc_did_warn = true; | ||
| std::cerr | ||
| << "WARNING: huge allocation count exceeds HWMALLOC_NUM_HUGE_ALLOC_WARN_THERESHOLD=" |
There was a problem hiding this comment.
| << "WARNING: huge allocation count exceeds HWMALLOC_NUM_HUGE_ALLOC_WARN_THERESHOLD=" | |
| << "WARNING: huge allocation count exceeds HWMALLOC_NUM_HUGE_ALLOC_WARN_THRESHOLD=" |
| detail::get_env_size_t("HWMALLOC_TINY_SEGMENT_SIZE", (1u << 16)), // 64KiB | ||
| detail::get_env_size_t("HWMALLOC_SMALL_SEGMENT_SIZE", (1u << 16)), // 64KiB | ||
| detail::get_env_size_t("HWMALLOC_LARGE_SEGMENT_SIZE", (1u << 21)), // 2MiB | ||
| detail::get_env_size_t("HWMALLOC_NUM_HUGE_ALLOC_WARN_THERESHOLD", 10) |
There was a problem hiding this comment.
Separate comment for default value: Probably it's good to warn immediately when going over num_reserve_segments? I.e. set the threshold to 1 or 0?
| detail::get_env_size_t("HWMALLOC_TINY_SEGMENT_SIZE", (1u << 16)), // 64KiB | ||
| detail::get_env_size_t("HWMALLOC_SMALL_SEGMENT_SIZE", (1u << 16)), // 64KiB | ||
| detail::get_env_size_t("HWMALLOC_LARGE_SEGMENT_SIZE", (1u << 21)), // 2MiB | ||
| detail::get_env_size_t("HWMALLOC_NUM_HUGE_ALLOC_WARN_THERESHOLD", 10) |
There was a problem hiding this comment.
And third comment for naming: the warning should likely happen for any allocation size, not just the huge allocations.
| ::setenv("HWMALLOC_TINY_SEGMENT_SIZE", "16384", 1); | ||
| ::setenv("HWMALLOC_SMALL_SEGMENT_SIZE", "bar", 1); | ||
| ::setenv("HWMALLOC_LARGE_SEGMENT_SIZE", "4194304", 1); | ||
|
|
There was a problem hiding this comment.
Probably good to check that the threshold config option is taken into account here.
| ::setenv("HWMALLOC_LARGE_LIMIT", "131072", 1); | ||
| ::setenv("HWMALLOC_TINY_SEGMENT_SIZE", "16384", 1); | ||
| ::setenv("HWMALLOC_SMALL_SEGMENT_SIZE", "32768", 1); | ||
| ::setenv("HWMALLOC_LARGE_SEGMENT_SIZE", "262144", 1); |
There was a problem hiding this comment.
Good to check the threshold config option here.
| inline constexpr std::size_t num_tiny_heaps_default = 16u; | ||
| inline constexpr std::size_t num_small_heaps_default = 5u; | ||
| inline constexpr std::size_t num_large_heaps_default = 9u; | ||
|
|
There was a problem hiding this comment.
I would add a test here for the threshold value.
2 participants
WIP