MySQL Bugs: #106672: Build with -DWITH_VALGRIND=1 allocates way too much due to "x1.5" MEMROOT growth

Bug #106672	Build with -DWITH_VALGRIND=1 allocates way too much due to "x1.5" MEMROOT growth
Submitted:	8 Mar 2022 14:11	Modified:	1 Apr 2022 19:41
Reporter:	Guilhem Bichot	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Data Types	Severity:	S6 (Debug Builds)
Version:	8.0	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
Here:
https://github.com/mysql/mysql-server/blob/8.0/mysys/my_alloc.cc

void *MEM_ROOT::AllocSlow(size_t length) {
  DBUG_TRACE;
  DBUG_PRINT("enter", ("root: %p", this));

  // We need to allocate a new block to satisfy this allocation;
  // otherwise, the fast path in Alloc() would not have sent us here.
  // We plan to allocate a block of <block_size> bytes; see if that
  // would be enough or not.
  if (length >= m_block_size || MEM_ROOT_SINGLE_CHUNKS) {

When building with -DWITH_DEBUG=1 -DWITH_VALGRIND=1, MEM_ROOT_SINGLE_CHUNKS is set to 1. So when we ask for 10 bytes to MEMROOT, it allocates a new block. This is done by:

MEM_ROOT::Block *MEM_ROOT::AllocBlock(size_t wanted_length,
                                      size_t minimum_length) {
 ...
  // Make the default block size 50% larger next time.
  // This ensures O(1) total mallocs (assuming Clear() is not called).
  m_block_size += m_block_size / 2;
  return new_block;
}

so here, we ask for 10 bytes, we get them (in the "...") and m_block_size is multiplied by 1.5. And the next time we ask for 10 bytes, again it will be multiplied. So we can quickly reach very high m_block_size, which in the end makes MEMROOT stop working. For example, such call could fail:

bool MEM_ROOT::ForceNewBlock(size_t minimum_length) {
  std::pair<Block *, size_t> block_and_length =
      AllocBlock(/*wanted_length=*/ALIGN_SIZE(m_block_size),
                 minimum_length);  // Will modify block_size.
as it will try to alloc the large value.

How to repeat:
No testcase, but I've seen it happen in gdb while debugging, and from code review it looks clear.

Suggested fix:
I think that this line
  // Make the default block size 50% larger next time.
  // This ensures O(1) total mallocs (assuming Clear() is not called).
  m_block_size += m_block_size / 2;
should be prefixed with
if (!MEM_ROOT_SINGLE_CHUNKS).

Salut Guilhem, mon ami,

Thank you for your bug report.

We have analysed your report and indeed concluded that this is an error that would lead to memory fragmentation.

In short, we agree with your analysis.

Verified as reported.

This affects only debug builds.

Posted by developer:
 
Note that the cmake option WITH_VALGRIND is mostly obsolete.
It was used earlier to enable or disable code of the sort:
"I am smarter than the compiler, or smarter than purify/valgrind"
That code was remove some eight years ago.

The only thing WITH_VALGRIND does these days is to enable some
VALGRIND_MAKE_MEM_DEFINED etc. in InnoDB.

I recommend running valgrind with your normal Debug or RelWithDebInfo builds.

Posted by developer:
 
5.7 code is completely different.

Posted by developer:
 
For normal allocation, we will never call ForceNewBlock.
This code however, may do so:
sql/iterators/hash_join_buffer.cc:    m_mem_root.ForceNewBlock(required_value_bytes);
sql/iterators/hash_join_buffer.cc:    m_mem_root.ForceNewBlock(required_key_bytes);

Fixed in MySQL 8.0.30.

Debugging only, no user-facing changes to document.

Closed.

Thank you, Jon.