Reolving mediawiki runtime and score variations (facebookresearch#158)

Summary:
Pull Request resolved: facebookresearch#158

Both Mediawiki and Mediawiki_mini were exhibiting variations in execution time and achieved scores, particularly on ARM machines. Such variations in benchmarking can lead to incorrect conclusions for anyone conducting experiments. Since one of the purposes of DCPerf_mini is to be used in machine health checks, and a key requirement for this is to maintain variation below 3%, it is essential to address this inconsistency.
This diff addresses the variation by resolving three sources of randomness:
Randomness in Load Generation: By applying a fixed seed, we can control the randomness in load generation. Since the seed affects the score, this diff allows the DCPerf user to configure the seed or use the current time (os.time) as the seed if a negative seed is provided.
Random and Prolonged Warmup Times: This issue is resolved by addressing the scenario where overlapping translation frequencies and the duration of each load generated for warmup result in a pending translation every time the server checks the warmup status. By distinguishing when the server is waiting for a pending translation, we can choose to wait rather than initiate a new load.
Benchmark Execution Method: Running the benchmark directly from a script, as opposed to executing the command from a subprocess in Python, results in a simpler trace being observed by HHVM. This leads to more efficient translation.

Differential Revision: D77906847

Author: marziehlenjaniMeta

benchpress/config/jobs.yml

@@ -33,6 +33,7 @@
 - benchmark: oss_performance_mediawiki
   name: oss_performance_mediawiki_mini
   description: Default run for oss_performance_mediawiki
+  execute_cmd_from_file: true
   args:
     - '-r/usr/local/hphpi/legacy/bin/hhvm'
     - '-nnginx'
@@ -49,21 +50,24 @@
     - '--shorten-health-check'
     - '--skip-single-request-warmup'
     - '--skip-sleep-between-warmups'
-    - '--hhvm-extra-arguments="-vEval.JitRetranslateAllSeconds={warmup_seconds}"'
+    - '--hhvm-extra-arguments="-vEval.JitRetranslateAllSeconds={translation_interval_seconds}"'
     - '--num-multi-req-warmups={num_multi_req_warmups}'
+    - '--no-load-if-pending-translate'
     - '--first-multi-warmup-duration={first_multi_warmup_duration}'
     - '--subseq-multi-warmup-duration={subseq_multi_warmup_duration}'
+    - '--load-gen-seed={load_generator_seed}'
     - '{extra_args}'
   vars:
     - 'load_generator=wrk'
     - 'lg_path=benchmarks/oss_performance_mediawiki/wrk/wrk'
     - 'duration=4s'
-    - 'timeout=20s'
-    - 'warmup_seconds=5'
-    - 'first_multi_warmup_duration=10'
-    - 'subseq_multi_warmup_duration=20'
+    - 'timeout=60s'
+    - 'translation_interval_seconds=5'
+    - 'first_multi_warmup_duration=25' # long enough to get a high score
+    - 'subseq_multi_warmup_duration=5'
     - 'temp_dir=default_no_temp_dir'
-    - 'num_multi_req_warmups=1'
+    - 'num_multi_req_warmups=-1' # -1 means the warmup status will define the number of iterations
+    - 'load_generator_seed=1000'
     - 'extra_args='
   hooks:
     - hook: copymove

benchpress/lib/job.py

@@ -6,6 +6,7 @@
 
 import errno
 import logging
+import os
 import subprocess
 import sys
 import tempfile
@@ -89,6 +90,7 @@ def __init__(self, job_config, benchmark_config, toolchain_config) -> None:
         # if this option is a string, the output will be written to the file
         # named by this value
         self.tee_output = job_config.get("tee_output", False)
+        self.execute_cmd_from_file = job_config.get("execute_cmd_from_file", False)
 
         self.tags = formalize_tags([benchmark_config, job_config])
 
@@ -199,6 +201,22 @@ def dry_run(self, role=None, role_input=None):
         self.check_role(role, role_input)
         return get_safe_cmd([self.binary] + self.args)
 
+    def get_file_based_cmd(self, role=None, role_input=None, fp=None):
+        """Dump the run command in a file and execute it."""
+        logger.info('Starting "{}"'.format(self.name))
+        cmd = self.dry_run(role, role_input)
+        click.echo("Job execution command: {}".format(cmd))
+        # add string to cmd so that it dumps the stdout and strerr to different files
+        # cmd = cmd + " > benchpress_run_output.txt 2> benchpress_run_error.txt"
+
+        # write the command to a file
+        fp.write(b"#!/bin/bash\n")
+        fp.write((" ".join(cmd)).encode("utf-8"))
+        fp.write(b"\n")  # Add a newline at the end
+        fp.flush()  # Ensure the file is written to disk
+        os.chmod(fp.name, 0o755)
+        return [str(fp.name)]
+
     def run(self, role=None, role_input=None):
         """Run the benchmark and return the metrics that are reported.
         check if user type role correctly
@@ -207,14 +225,25 @@ def run(self, role=None, role_input=None):
 
         try:
             logger.info('Starting "{}"'.format(self.name))
-            cmd = get_safe_cmd([self.binary] + self.args)
-            click.echo("Job execution command: {}".format(cmd))
-            process = subprocess.Popen(
-                cmd,
-                stdout=subprocess.PIPE,
-                stderr=subprocess.PIPE,
-                text=True,
-            )
+            if self.execute_cmd_from_file:
+                fp = tempfile.NamedTemporaryFile(delete=False)
+                cmd = self.get_file_based_cmd(role, role_input, fp)
+                fp.close()  # Close the file before executing it
+                process = subprocess.Popen(
+                    cmd,
+                    stdout=subprocess.PIPE,
+                    stderr=subprocess.PIPE,
+                    text=True,
+                )
+            else:
+                cmd = get_safe_cmd([self.binary] + self.args)
+                click.echo("Job execution command: {}".format(cmd))
+                process = subprocess.Popen(
+                    cmd,
+                    stdout=subprocess.PIPE,
+                    stderr=subprocess.PIPE,
+                    text=True,
+                )
             stdout_storage = tempfile.TemporaryFile(
                 mode="w+",
                 encoding="utf-8",

packages/mediawiki/0007-oss-performance-more-warmup-options.diff

@@ -41,66 +41,74 @@ index 420014d..d8e45df 100644
    public function queueEmpty(): bool { return true; }
  }
 diff --git a/base/PerfOptions.php b/base/PerfOptions.php
-index add42c7..7603fdf 100644
+index add42c7..dfe40a6 100644
 --- a/base/PerfOptions.php
 +++ b/base/PerfOptions.php
-@@ -58,6 +58,15 @@ final class PerfOptions {
+@@ -58,6 +58,17 @@ final class PerfOptions {
    public bool $noTimeLimit = false;
    public bool $runAsRoot = false;
 
 +  // Options for shortening the execution time
 +  public bool $shortenHealthCheck = false;
 +  public bool $skipSingleRequestWarmup = false;
 +  public bool $skipSleepBetweenWarmups = false;
++  public bool $noLoadIfPendingTranslate = false;
 +  public int  $firstMultiWarmupDuration = 60;
 +  public int  $subseqMultiWarmupDuration = 10;
++  public int  $loadGenSeed = 1000;
 +  public int  $numMultiReqWarmups = -1;
 +
 +
    // Pause once benchmarking is complete to allow for manual inspection of the
    // HHVM or PHP process.
    public bool $waitAtEnd = false;
-@@ -105,6 +114,7 @@ final class PerfOptions {
+@@ -105,6 +116,7 @@ final class PerfOptions {
    public float $delayProcessLaunch; // secs to wait after start process
    public float $delayCheckHealth; // secs to wait before hit /check-health
 
 +
    //
    // Maximum wait times, as for example given to file_get_contents
    // or the configuration file for nginx.  These times may be truncated
-@@ -221,6 +231,12 @@ final class PerfOptions {
+@@ -221,6 +233,14 @@ final class PerfOptions {
        'memcached-threads:',
        'no-memcached', // do not use memcached (even if target supports it)
        'scale-out:',
 +      'shorten-health-check',
 +      'skip-single-request-warmup',
 +      'skip-sleep-between-warmups',
++      'no-load-if-pending-translate',
 +      'first-multi-warmup-duration:',
 +      'subseq-multi-warmup-duration:',
++      'load-gen-seed:',
 +      'num-multi-req-warmups:'
      };
      $targets = $this->getTargetDefinitions()->keys();
      $def->addAll($targets);
-@@ -312,6 +328,14 @@ final class PerfOptions {
+@@ -312,6 +332,18 @@ final class PerfOptions {
      $this->applyPatches = $this->getBool('apply-patches');
      $this->useMemcached = !$this->getBool('no-memcached');
 
 +    $this->shortenHealthCheck = $this->getBool('shorten-health-check');
 +    $this->skipSingleRequestWarmup = $this->getBool('skip-single-request-warmup');
 +    $this->skipSleepBetweenWarmups = $this->getBool('skip-sleep-between-warmups');
++    $this->noLoadIfPendingTranslate = $this->getBool('no-load-if-pending-translate');
 +    $this->firstMultiWarmupDuration = $this->getInt('first-multi-warmup-duration', 60);
 +    $this->subseqMultiWarmupDuration = $this->getInt('subseq-multi-warmup-duration', 10);
++    $this->loadGenSeed = $this->getInt('load-gen-seed', 1000);
++
++
 +    $this->numMultiReqWarmups = $this->getInt('num-multi-req-warmups', -1);
 +
 +
      $fraction = $this->getFloat('cpu-fraction', 1.0);
      if ($fraction !== 1.0) {
        $this->cpuBind = true;
 diff --git a/base/PerfRunner.php b/base/PerfRunner.php
-index 5201a2d..7c44632 100644
+index 5201a2d..478edd3 100644
 --- a/base/PerfRunner.php
 +++ b/base/PerfRunner.php
-@@ -219,27 +219,53 @@ final class PerfRunner {
+@@ -219,27 +219,71 @@ final class PerfRunner {
        exec($options->scriptBeforeWarmup);
      }
 
@@ -113,43 +121,61 @@ index 5201a2d..7c44632 100644
 
      if (!$options->skipWarmUp) {
 -      self::RunLoadGenerator(RequestModes::WARMUP_MULTI, $options, $target, $engines);
-+      self::RunLoadGenerator(RequestModes::WARMUP_MULTI, $options, $target, $engines, $options->firstMultiWarmupDuration);
++      self::RunLoadGenerator(RequestModes::WARMUP_MULTI,
++      $options, $target, $engines, $options->firstMultiWarmupDuration);
      } else {
 -      self::PrintProgress('Skipping multi request warmup');
 +      self::PrintProgress('Skipping first multi request warmup');
-     }
++    }
 +    if ($options->numMultiReqWarmups >= 0){
-+      self::PrintProgress('Deterministic warmpup with '.($options->numMultiReqWarmups). ' iterations');
++      self::PrintProgress('Deterministic warmpup with '
++      .($options->numMultiReqWarmups). ' iterations');
 +      for ($i = 0; $i < $options->numMultiReqWarmups; $i++){
-+        self::PrintProgress(' Performing '. ($i+1). 'th multi-request warmup iteration out of '.($options->numMultiReqWarmups). ' iterations');
++        self::PrintProgress(' Performing '. ($i+1).
++        'th multi-request warmup iteration out of '.
++        ($options->numMultiReqWarmups));
 +        if (!$options->skipSleepBetweenWarmups){
 +          sleep(3);
 +        }
-+        self::RunLoadGenerator(RequestModes::WARMUP_MULTI, $options, $target, $engines, $options->subseqMultiWarmupDuration);
-+
++        self::RunLoadGenerator(RequestModes::WARMUP_MULTI,
++         $options, $target, $engines, $options->subseqMultiWarmupDuration);
 +        $status_message = null;
 +        if (self::NeedsRetranslatePause($engines, inout $status_message)){
-+          self::PrintProgress('Server is not done warming up. Status: ' . ($status_message ?? 'Unknown'));
++          self::PrintProgress("Server is not done warming up.\n Status: " .
++          ($status_message ?? 'Unknown'));
 +        }else{
 +          self::PrintProgress('Server is done warming up.');
 +        }
 +      }
- 
--    while (!$options->skipWarmUp && self::NeedsRetranslatePause($engines)) {
--      self::PrintProgress('Extending warmup, server is not done warming up.');
--      sleep(3);
--      self::RunLoadGenerator(RequestModes::WARMUP_MULTI, $options, $target, $engines, 10);
-+    }
++
+     }
 +    else{
 +      while (!$options->skipWarmUp) {
 +        $status_message = null;
 +        if (self::NeedsRetranslatePause($engines, inout $status_message)) {
-+          self::PrintProgress('Extending warmup, server is not done warming up. Status: ' . ($status_message ?? 'Unknown'));
++          self::PrintProgress("Extending warmup ... \n server is not done warming up.
++           \n Status: " . ($status_message ?? 'Unknown'));
 +          if (!$options->skipSleepBetweenWarmups){
 +            sleep(3);
 +          }
-+          self::RunLoadGenerator(RequestModes::WARMUP_MULTI, $options, $target, $engines, $options->subseqMultiWarmupDuration);
-+        } else {
++          # if $status_message contains Waiting on retranslateAll(),
++          # we might only need to wait a bit longer
++          if ($options->noLoadIfPendingTranslate &&
++           $status_message !== null &&
++            strpos($status_message, "Waiting on retranslateAll()") !== false){
++            self::PrintProgress('no new load if pending translate is set,
++             waiting for retranslateAll');
++          }else{
++            self::RunLoadGenerator(RequestModes::WARMUP_MULTI, $options, $target, $engines,
++             $options->subseqMultiWarmupDuration);
++          }
+ 
+-    while (!$options->skipWarmUp && self::NeedsRetranslatePause($engines)) {
+-      self::PrintProgress('Extending warmup, server is not done warming up.');
+-      sleep(3);
+-      self::RunLoadGenerator(RequestModes::WARMUP_MULTI, $options, $target, $engines, 10);
++        }else {
++          self::PrintProgress('Server is done warming up');
 +          break;
 +        }
 +      }
@@ -163,7 +189,7 @@ index 5201a2d..7c44632 100644
        sleep(10);
      }
 
-@@ -327,9 +353,9 @@ final class PerfRunner {
+@@ -327,9 +371,9 @@ final class PerfRunner {
      return $combined_stats;
    }
 

packages/mediawiki/Wrk.php

@@ -26,6 +26,21 @@ public function __construct(
     } else {
       $this->logfile = tempnam($options->tempDir, 'wrk_warmup');
     }
+    if ($this->options->loadGenSeed > 0) {
+      # Create template path by adding .template suffix
+      $template_path = $this->script . '.template';
+
+      # Read the content from the template file
+      $script_content = file_get_contents($template_path);
+
+      # Find and replace the math.randomseed line with our custom seed value
+      $pattern = '/math\.randomseed\s*\(\s*os\.time\s*\(\s*\)\s*\)/';
+      $replacement = "math.randomseed(".((string)$this->options->loadGenSeed).")";
+      $modified_content = preg_replace($pattern, $replacement, $script_content);
+
+      # Write the modified content to the script file
+      file_put_contents($this->script, $modified_content);
+    }
   }
 
   public function start(): void {

packages/mediawiki/install_oss_performance_mediawiki.sh

@@ -140,7 +140,7 @@ git apply --check "${TEMPLATES_DIR}/0005-scale-out-memcached.diff" \
 # Copy wrk related stuff
 cp "${TEMPLATES_DIR}/Wrk.php" ./base/Wrk.php
 cp "${TEMPLATES_DIR}/WrkStats.php" ./base/WrkStats.php
-cp "${TEMPLATES_DIR}/multi-request-txt.lua" ./scripts/multi-request-txt.lua
+cp "${TEMPLATES_DIR}/multi-request-txt.lua" ./scripts/multi-request-txt.lua.template
 
 # shellcheck disable=SC2046
 curl -O https://getcomposer.org/installer

packages/mediawiki/multi-request-txt.lua

@@ -6,7 +6,7 @@
 -- Module instantiation
 -- Initialize the pseudo random number generator
 -- Resource: http://lua-users.org/wiki/MathLibraryTutorial
-math.randomseed(1000)
+math.randomseed(os.time())
 math.random(); math.random(); math.random()
 
 -- Shuffle array