[MDEV-22203] WSREP_ON is unnecessarily expensive to evaluate Created: 2020-04-09  Updated: 2020-04-27  Resolved: 2020-04-24

Status: Closed
Project: MariaDB Server
Component/s: Galera
Affects Version/s: 10.1.17, 10.2.2, 10.3.0, 10.4.0, 10.5.0
Fix Version/s: 10.1.45, 10.2.32, 10.3.23, 10.4.13, 10.5.3

Type: Bug Priority: Major
Reporter: Marko Mäkelä Assignee: Jan Lindström (Inactive)
Resolution: Fixed Votes: 1
Labels: performance

Attachments: File MDEV-22203.ods     PNG File sysbench_read_only.png     PNG File sysbench_read_write.png     PNG File sysbench_update_index.png     PNG File sysbench_update_non_index.png     PNG File sysbench_write_only.png    
Issue Links:
Relates
relates to MDEV-7962 wsrep_on() takes 0.14% in OLTP RO Closed
relates to MDEV-16405 Merge Galera 4 changes from mariaDB_w... Closed
relates to MDEV-16799 Test wsrep.variables crash at sql_cla... Closed

 Description   

The macro WSREP_ON used to be defined simply as an alias the global Boolean variable global_system_variables.wsrep_on. A more complex variant that invokes strcmp was introduced, in MDEV-16799 renamed to WSREP_ON_NEW, and related to MDEV-16405 (Galera 4) in 10.4.2 the more efficient macro WSREP_ON was replaced altogether.

The following looks grossly inefficient:

#define WSREP_ON                         \
 
  ((global_system_variables.wsrep_on) && \
 
   wsrep_provider                     && \
 
   strcmp(wsrep_provider, WSREP_NONE))
 
 
 
/* use xxxxxx_NNULL macros when thd pointer is guaranteed to be non-null to
 
 * avoid compiler warnings (GCC 6 and later) */
 
#define WSREP_NNULL(thd) \
 
  (thd->variables.wsrep_on && WSREP_ON)
 
 
 
#define WSREP(thd) \
 
  (thd && WSREP_NNULL(thd))

Why does the Boolean predicate WSREP_ON have to be so complex? We should be able to compute one global Boolean variable, something like this:

extern my_bool WSREP_ON;
 
#define WSREP_NNULL(thd) (WSREP_ON && thd->variables.wsrep_on)
 
#define WSREP(thd) (thd && WSREP_NNULL(thd))

The Boolean variable WSREP_ON would be updated by the update callback functions that would be attached to global_system_variables.wsrep_on and wsrep_provider.

There are rather many checks for WSREP_ON, so this could affect performance, especially starting with MariaDB Server 10.4. I feel that this is important, because the code is being invoked even when Galera is not being used.

MDEV-7962 was filed for performance regressions due to a similar check that is implemented in a non-inlined function wsrep_on(). Could we replace that one with an efficient implementation of the macro WSREP_ON?

 Comments   
Comment by Axel Schwenke [ 2020-04-21 ]

I ran some standard sysbench workloads on MariaDB 10.4.12 with WSREP compiled in or not compiled in. As expected, disabling WSREP gives a small performance improvement. Also as expected, this improvement is bigger for write-intensive workload. The improvement is however small, in the order of 2%.

Comment by Michael Widenius [ 2020-04-22 ]

Axel, did you test with and without binary logging?
In any case, to make performant code, all code should have a decent quality without 'obvious' bottlenecks.
The current code is an obvious bottleneck and anyone who would look at it would believe that we don't care about performance, which is definitely not the case.
Code like this should never be produced under any circumstances and especially not for something that is called many times during one statement execution.

The proper and only acceptable fix is to change the code to use a Boolean variable for the check.
If possible, I would argue that this variable is not allowed to change during a running transactions. If this is true, then the current state of wsrep should be stored in thd at start of transaction and the WSREP macros that are using thd should ONLY look at the thd during the execution of the transaction.

Comment by Axel Schwenke [ 2020-04-22 ]

monty : this test was without binlog. But I can easily run it again with binlog enabled.

Comment by Axel Schwenke [ 2020-04-22 ]

I updated the attachments with results from the benchmark; rerun with binlogging enabled. The general numbers dropped with binlogging enabled (as expected). It also looks like the difference between WITH_WSREP=1 and WITH_WSREP=0 gets smaller when binlogging is enabled. This is also expected. Binlogging adds latency to query processing (at least while waiting for the disk), so any constant overhead (we expect such overhead for WSREP=On checks) will look smaller in relation to the overall processing time.

Comment by Marko Mäkelä [ 2020-04-26 ]

Before 10.4, WSREP only reads a global variable, and WSREP_ON_NEW was always unused. MDEV-16799 essentially reverted the definition of WSREP_ON to what it used to be.
I will port some cleanup (and the addition of an unlikely() hint to 10.1. There is not much more than can be done there.

Generated at Thu Feb 08 09:12:59 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.