[MDEV-22931] mtr_t::mtr_t() allocates some memory Created: 2020-06-18 Updated: 2020-07-02 Resolved: 2020-06-19 |
|
| Status: | Closed |
| Project: | MariaDB Server |
| Component/s: | Storage Engine - InnoDB |
| Affects Version/s: | 10.5 |
| Fix Version/s: | 10.5.5 |
| Type: | Bug | Priority: | Major |
| Reporter: | Vladislav Vaintroub | Assignee: | Thirunarayanan Balathandayuthapani |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | performance, regression | ||
| Attachments: |
2020-06-18 (5).png
2020-06-18 (6).png
|
||||||||||||||||
| Issue Links: |
|
||||||||||||||||
| Description |
|
This was a rather surprising find while profiling. 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) |
| Comments |
| Comment by Marko Mäkelä [ 2020-06-18 ] | |||||||||
|
This might be due to the m_freed_ranges that 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. | |||||||||
| Comment by Vladislav Vaintroub [ 2020-06-18 ] | |||||||||
|
| |||||||||
| Comment by Marko Mäkelä [ 2020-06-18 ] | |||||||||
|
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. | |||||||||
| Comment by Eugene Kosov (Inactive) [ 2020-06-18 ] | |||||||||
|
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> | |||||||||
| Comment by Marko Mäkelä [ 2020-06-19 ] | |||||||||
|
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). | |||||||||
| Comment by Matthias Leich [ 2020-06-19 ] | |||||||||
|
origin/bb-10.5-thiru 0a3c6585257f988e1fbf01a2d51284043a5ab7a6 2020-06-19T15:57:36+05:30 |
