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

WSREP_ON is unnecessarily expensive to evaluate

Details

    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?

      Attachments

        1. MDEV-22203.ods
          54 kB
        2. sysbench_read_only.png
          sysbench_read_only.png
          5 kB
        3. sysbench_read_write.png
          sysbench_read_write.png
          6 kB
        4. sysbench_update_index.png
          sysbench_update_index.png
          6 kB
        5. sysbench_update_non_index.png
          sysbench_update_non_index.png
          6 kB
        6. sysbench_write_only.png
          sysbench_write_only.png
          6 kB

        Issue Links

          relates to

          Bug - A problem which impairs or prevents the functions of the product. MDEV-7962 wsrep_on() takes 0.14% in OLTP RO

          • Major - Major loss of function.
          • Closed

          Task - A task that needs to be done. MDEV-16405 Merge Galera 4 changes from mariaDB_wsrep to 10.4

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

          Bug - A problem which impairs or prevents the functions of the product. MDEV-16799 Test wsrep.variables crash at sql_class.cc:639 thd_get_ha_data

          • Blocker - Blocks development and/or testing work, production could not run.
          • Closed

          Activity

            Ascending order - Click to sort in descending order
            axel Axel Schwenke added a comment -

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

            axel Axel Schwenke added a comment - 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%.
            monty Michael Widenius added a comment -

            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.

            monty Michael Widenius added a comment - 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.
            axel Axel Schwenke added a comment -

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

            axel Axel Schwenke added a comment - monty : this test was without binlog. But I can easily run it again with binlog enabled.
            axel Axel Schwenke added a comment -

            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.

            axel Axel Schwenke added a comment - 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.
            marko Marko Mäkelä added a comment -

            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.

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

            People

              jplindst Jan Lindström (Inactive)
              marko Marko Mäkelä
              Votes:
              1 Vote for this issue
              Watchers:
              5 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.