Merge lp:~akopytov/percona-server/bug1163262-5.5 into lp:percona-server/5.5

Proposed by Alexey Kopytov
Status: Merged
Approved by: Laurynas Biveinis
Approved revision: no longer in the source branch.
Merged at revision: 492
Proposed branch: lp:~akopytov/percona-server/bug1163262-5.5
Merge into: lp:percona-server/5.5
Diff against target: 137 lines (+56/-4)
4 files modified
Percona-Server/storage/innobase/include/buf0flu.ic (+1/-1)
Percona-Server/storage/innobase/include/mtr0mtr.h (+2/-0)
Percona-Server/storage/innobase/include/mtr0mtr.ic (+21/-0)
Percona-Server/storage/innobase/mtr/mtr0mtr.c (+32/-3)
To merge this branch: bzr merge lp:~akopytov/percona-server/bug1163262-5.5
Reviewer Review Type Date Requested Status
Laurynas Biveinis (community) Approve
Registry Administrators Pending
Review via email: mp+156630@code.launchpad.net

Description of the change

    Bug #1163262: Unnecessary log_flush_order_mutex acquisition

    log_sys->log_flush_order_mutex was introduced as an optimization to
    reduce contention on log_sys->mutex. On a mini-transaction commit
    InnoDB adds modified pages to the flush list and uses
    log_sys->log_flush_order_mutex to make sure pages are added to the
    flush list in the correct LSN order.

    One thing that was handled inefficiently wrt. that mutex is that it was
    acquired even if no modifications from a mini-transaction had to be
    added to flush list, i.e. when an mtr has only modified dirty pages
    (if any). This unnecessarily increased contention on
    log_sys->log_flush_order_mutex and consequently on log_sys->mutex in
    write-intensive workloads, because the former is acquired with the
    latter locked.

    The problem is fixed in MySQL 5.6 with the following revision:
    http://lists.mysql.com/commits/134048

    This is a backport of that change to Percona Server 5.5.

http://jenkins.percona.com/view/PS%205.5/job/percona-server-5.5-param/712/

To post a comment you must log in.
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

The patch introduces a new function mtr_block_dirtied(). Its definition is provided in mtr0log.c, it's a small function, and has only one caller in in mtr_memo_push(), which is defined in mtr0mtr.ic. Surely this means that we want mtr_block_dirtied() defined as a static inline function in mtr0mtr.ic as well?

review: Needs Information
Revision history for this message
Alexey Kopytov (akopytov) wrote :

I noticed that too, but it's a backport of the upstream 5.6 code and it's still defined like that in upstream LP. So I decided to keep things simple and not diverge from the upstream code.

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

I wonder if the corresponding upstream bug to point this out to them would be "a misaligned source comment" kind of bug or if it's worthwhile to report it.

In any case, looks good, and we can wait for mtr_block_dirtied() call overhead to show up in profiles before diverging.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Percona-Server/storage/innobase/include/buf0flu.ic'
--- Percona-Server/storage/innobase/include/buf0flu.ic 2012-05-10 07:49:14 +0000
+++ Percona-Server/storage/innobase/include/buf0flu.ic 2013-04-02 16:21:25 +0000
@@ -70,7 +70,7 @@
7070
71 ut_ad(!buf_pool_mutex_own(buf_pool));71 ut_ad(!buf_pool_mutex_own(buf_pool));
72 ut_ad(!buf_flush_list_mutex_own(buf_pool));72 ut_ad(!buf_flush_list_mutex_own(buf_pool));
73 ut_ad(log_flush_order_mutex_own());73 ut_ad(!mtr->made_dirty || log_flush_order_mutex_own());
7474
75 ut_ad(mtr->start_lsn != 0);75 ut_ad(mtr->start_lsn != 0);
76 ut_ad(mtr->modifications);76 ut_ad(mtr->modifications);
7777
=== modified file 'Percona-Server/storage/innobase/include/mtr0mtr.h'
--- Percona-Server/storage/innobase/include/mtr0mtr.h 2012-02-17 09:52:51 +0000
+++ Percona-Server/storage/innobase/include/mtr0mtr.h 2013-04-02 16:21:25 +0000
@@ -375,6 +375,8 @@
375 ibool modifications;375 ibool modifications;
376 /* TRUE if the mtr made modifications to376 /* TRUE if the mtr made modifications to
377 buffer pool pages */377 buffer pool pages */
378 ibool made_dirty;/*!< TRUE if mtr has made at least
379 one buffer pool page dirty */
378 ulint n_log_recs;380 ulint n_log_recs;
379 /* count of how many page initial log records381 /* count of how many page initial log records
380 have been written to the mtr log */382 have been written to the mtr log */
381383
=== modified file 'Percona-Server/storage/innobase/include/mtr0mtr.ic'
--- Percona-Server/storage/innobase/include/mtr0mtr.ic 2012-03-20 09:44:51 +0000
+++ Percona-Server/storage/innobase/include/mtr0mtr.ic 2013-04-02 16:21:25 +0000
@@ -29,6 +29,17 @@
29#endif /* !UNIV_HOTBACKUP */29#endif /* !UNIV_HOTBACKUP */
30#include "mach0data.h"30#include "mach0data.h"
3131
32/***************************************************//**
33Checks if a mini-transaction is dirtying a clean page.
34@return TRUE if the mtr is dirtying a clean page. */
35UNIV_INTERN
36ibool
37mtr_block_dirtied(
38/*==============*/
39 const buf_block_t* block) /*!< in: block being x-fixed */
40 __attribute__((nonnull,warn_unused_result));
41
42
32/***************************************************************//**43/***************************************************************//**
33Starts a mini-transaction. */44Starts a mini-transaction. */
34UNIV_INLINE45UNIV_INLINE
@@ -47,6 +58,7 @@
47 mtr->inside_ibuf = FALSE;58 mtr->inside_ibuf = FALSE;
48 mtr->n_log_recs = 0;59 mtr->n_log_recs = 0;
49 mtr->n_freed_pages = 0;60 mtr->n_freed_pages = 0;
61 mtr->made_dirty = FALSE;
5062
51 ut_d(mtr->state = MTR_ACTIVE);63 ut_d(mtr->state = MTR_ACTIVE);
52 ut_d(mtr->magic_n = MTR_MAGIC_N);64 ut_d(mtr->magic_n = MTR_MAGIC_N);
@@ -65,6 +77,15 @@
65 dyn_array_t* memo;77 dyn_array_t* memo;
66 mtr_memo_slot_t* slot;78 mtr_memo_slot_t* slot;
6779
80 /* If this mtr has x-fixed a clean page then we set
81 the made_dirty flag. This tells us if we need to
82 grab log_flush_order_mutex at mtr_commit so that we
83 can insert the dirtied page to the flush list. */
84 if (type == MTR_MEMO_PAGE_X_FIX && !mtr->made_dirty) {
85 mtr->made_dirty =
86 mtr_block_dirtied((const buf_block_t *)object);
87 }
88
68 ut_ad(object);89 ut_ad(object);
69 ut_ad(type >= MTR_MEMO_PAGE_S_FIX);90 ut_ad(type >= MTR_MEMO_PAGE_S_FIX);
70 ut_ad(type <= MTR_MEMO_X_LOCK);91 ut_ad(type <= MTR_MEMO_X_LOCK);
7192
=== modified file 'Percona-Server/storage/innobase/mtr/mtr0mtr.c'
--- Percona-Server/storage/innobase/mtr/mtr0mtr.c 2012-05-10 07:49:14 +0000
+++ Percona-Server/storage/innobase/mtr/mtr0mtr.c 2013-04-02 16:21:25 +0000
@@ -37,6 +37,25 @@
3737
38#ifndef UNIV_HOTBACKUP38#ifndef UNIV_HOTBACKUP
39# include "log0recv.h"39# include "log0recv.h"
40
41/***************************************************//**
42Checks if a mini-transaction is dirtying a clean page.
43@return TRUE if the mtr is dirtying a clean page. */
44UNIV_INTERN
45ibool
46mtr_block_dirtied(
47/*==============*/
48 const buf_block_t* block) /*!< in: block being x-fixed */
49{
50 ut_ad(buf_block_get_state(block) == BUF_BLOCK_FILE_PAGE);
51 ut_ad(block->page.buf_fix_count > 0);
52
53 /* It is OK to read oldest_modification because no
54 other thread can be performing a write of it and it
55 is only during write that the value is reset to 0. */
56 return(block->page.oldest_modification == 0);
57}
58
40/*****************************************************************//**59/*****************************************************************//**
41Releases the item in the slot given. */60Releases the item in the slot given. */
42static61static
@@ -125,7 +144,7 @@
125 buf_block_t* block = (buf_block_t*) slot->object;144 buf_block_t* block = (buf_block_t*) slot->object;
126145
127#ifdef UNIV_DEBUG146#ifdef UNIV_DEBUG
128 ut_ad(log_flush_order_mutex_own());147 ut_ad(!mtr->made_dirty || log_flush_order_mutex_own());
129#endif /* UNIV_DEBUG */148#endif /* UNIV_DEBUG */
130 buf_flush_note_modification(block, mtr);149 buf_flush_note_modification(block, mtr);
131 }150 }
@@ -225,7 +244,15 @@
225 mtr->end_lsn = log_close();244 mtr->end_lsn = log_close();
226245
227func_exit:246func_exit:
228 log_flush_order_mutex_enter();247
248 /* No need to acquire log_flush_order_mutex if this mtr has
249 not dirtied a clean page. log_flush_order_mutex is used to
250 ensure ordered insertions in the flush_list. We need to
251 insert in the flush_list iff the page in question was clean
252 before modifications. */
253 if (mtr->made_dirty) {
254 log_flush_order_mutex_enter();
255 }
229256
230 /* It is now safe to release the log mutex because the257 /* It is now safe to release the log mutex because the
231 flush_order mutex will ensure that we are the first one258 flush_order mutex will ensure that we are the first one
@@ -236,7 +263,9 @@
236 mtr_memo_note_modifications(mtr);263 mtr_memo_note_modifications(mtr);
237 }264 }
238265
239 log_flush_order_mutex_exit();266 if (mtr->made_dirty) {
267 log_flush_order_mutex_exit();
268 }
240}269}
241#endif /* !UNIV_HOTBACKUP */270#endif /* !UNIV_HOTBACKUP */
242271

Subscribers

People subscribed via source and target branches