Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-22931

mtr_t::mtr_t() allocates some memory

Details

    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

          Bug - A problem which impairs or prevents the functions of the product. MDEV-22970 Possible corruption of page_compressed tables, or when scrubbing is enabled

          • Critical - Crashes, loss of data, severe memory leak.
          • Closed
          is caused by

          Bug - A problem which impairs or prevents the functions of the product. MDEV-8139 Fix scrubbing

          • Major - Major loss of function.
          • Closed

          Activity

            Ascending order - Click to sort in descending order
            marko Marko Mäkelä added a comment -

            This might be due to the m_freed_ranges that MDEV-8139 added, 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.

            marko Marko Mäkelä added a comment - This might be due to the m_freed_ranges that MDEV-8139 added, 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.
            wlad Vladislav Vaintroub added a comment - 

            *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)

            wlad Vladislav Vaintroub added a comment - *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)
            marko Marko Mäkelä added a comment -

            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.

            marko Marko Mäkelä added a comment - 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.
            kevg Eugene Kosov (Inactive) added a comment -

            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>

            kevg Eugene Kosov (Inactive) added a comment - 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>
            marko Marko Mäkelä added a comment -

            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).

            marko Marko Mäkelä added a comment - 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).
            mleich Matthias Leich added a comment -

            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.

            mleich Matthias Leich added a comment - 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.

            People

              thiru Thirunarayanan Balathandayuthapani
              wlad Vladislav Vaintroub
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.