Details
-
Bug
-
Status: Closed (View Workflow)
-
Major
-
Resolution: Fixed
-
10.5
Description
This was a rather surprising find while profiling.
mtr_t::mtr_t allocates memory through construction of one of its members
This allocations accounts for 0.68% of CPU time (constructor itself for 0.77% of CPU time), this happens to be the largest contributor to the "operator new" on oltp_update_index benchmark with bufferpool larger than size of the data ( without doublewrite)
Attachments
Issue Links
- causes
-
-
- Closed
-
- is caused by
-
-
- Closed
-
Activity
*1 ucrtbase.dll!_malloc_base()
|
2 server.dll!operator new(unsigned __int64 size)
|
3 [Inline Frame] server.dll!std::allocator<std::_Tree_node<range_t,void *>>::allocate(const unsigned __int64)
|
4 [Inline Frame] server.dll!std::_Tree_node<range_t,void *>::_Buyheadnode(std::allocator<std::_Tree_node<range_t,void *>> &)
|
5 [Inline Frame] server.dll!std::_Tree<std::_Tset_traits<range_t,range_compare,std::allocator<range_t>,0>>::_Alloc_sentinel_and_proxy()
|
6 [Inline Frame] server.dll!std::_Tree<std::_Tset_traits<range_t,range_compare,std::allocator<range_t>,0>>::{ctor}(const range_compare &)
|
7 [Inline Frame] server.dll!std::set<range_t,range_compare,std::allocator<range_t>>::{ctor}()
|
8 server.dll!mtr_t::mtr_t()
|
9 server.dll!row_upd_clust_step(upd_node_t * node, que_thr_t * thr)
|
I think that the simplest fix is to add pointer indirection for m_freed_ranges, add an initializer expression = nullptr and allocate the object on demand. It should be very rare that a mini-transaction is freeing data pages.
We can probably assume (and assert) that when the object is allocated, it will always be nonempty.
Can a mini-transaction ever allocate a page that it has just freed? I do not see any debug assertion or handling of that. Could a debug assertion be added to mtr_t::init() that the page was not marked as freed?
Also, while fixing this, m_freed_ranges could be renamed to m_freed_pages, which is (almost) what the comment of mtr_t::add_freed_offset() is referring to.
Standard doesn't forbid allocating in default ctor. And Microsoft STL does so https://github.com/microsoft/STL/blob/master/stl/inc/xtree#L886 What a 'nice' implementation detail 
IMO, we should stick to std::unique_ptr<std::set>
I think that we can simply use pointer indirection, and only call delete in mtr_t::commit(). In that way, we can use memory leaks to our advantage in debugging. A missing call to mtr_t::commit() could then lead to an ASAN failure. If we used std::unique_ptr, such omissions would not lead to ASAN-reported leaks, but instead to difficult-to-find server hangs (when the page latches for the mini-transaction are never released).
origin/bb-10.5-thiru 0a3c6585257f988e1fbf01a2d51284043a5ab7a6 2020-06-19T15:57:36+05:30
containing the code of MDEV-22931 behaved well during the RQG test battery for broad
range functional coverage.
This might be due to the m_freed_ranges that
MDEV-8139added, or this is an older regression.In any case, we may want to introduce a separate object for read-only operations, and possibly we could avoid creating a mtr_t at all when an operation is only reading a small number of pages and could take care of releasing the page latches manually. For read-only operations, mtr_t::m_log and mtr_t::m_freed_ranges are totally unnecessary.
For 10.6, I have been thinking of replacing both mtr_t::m_log and mtr_t::m_memo with something like a std::unordered_map that maps buf_block_t* to log record snippets.