[libcxx][dsl] Reduce number of feature checks

[arichardson]

Summary

Even with caching of certain function calls the lit testsuite still takes
a while to start up. It turns out that most cache lookups are caused by
assertions inside the Feature class. This patch introduces a new
tryEnableIn() function. This allows us to only call isSupported() once
rather than currently tow or three times inside newconfig.py configure().

Another way of reducing the number of (potentially) expensive calls would
be to cache the value of the first isSupported() call (see D84055).

[ldionne]

What's the time gain like for doing this?

[arichardson]

In D89110#2322098, @ldionne wrote:

What's the time gain like for doing this?

Without this change:

Benchmark #1: ./bin/llvm-lit -v --no-execute projects/libcxx/test/libcxx/selftest/dsl/
  Time (mean ± σ):      2.621 s ±  0.029 s    [User: 1.455 s, System: 0.847 s]
  Range (min … max):    2.593 s …  2.688 s    10 runs

With the patch applied:

Benchmark #1: ./bin/llvm-lit -v --no-execute projects/libcxx/test/libcxx/selftest/dsl/
  Time (mean ± σ):      2.466 s ±  0.019 s    [User: 1.387 s, System: 0.779 s]
  Range (min … max):    2.435 s …  2.500 s    10 runs

So it appears to save ~150ms or about 6% of the total run time.

[arichardson]

In D89110#2322098, @ldionne wrote:

What's the time gain like for doing this?

Measuring with perf on a Linux machine I get the following without (rG0db08e59c9d2d3b004ea61f96d823edff283ed25):

Performance counter stats for './bin/llvm-lit -v --no-execute projects/libcxx/test/libcxx/selftest/dsl/' (10 runs):

      3001.646813      task-clock (msec)         #    0.971 CPUs utilized            ( +-  0.68% )
              530      context-switches          #    0.177 K/sec                    ( +-  0.20% )
               37      cpu-migrations            #    0.012 K/sec                    ( +-  8.62% )
          223,059      page-faults               #    0.074 M/sec                    ( +-  0.01% )
    5,421,406,209      cycles                    #    1.806 GHz                      ( +-  0.26% )
    5,856,892,943      instructions              #    1.08  insn per cycle           ( +-  0.01% )
    1,168,779,209      branches                  #  389.379 M/sec                    ( +-  0.01% )
       30,692,873      branch-misses             #    2.63% of all branches          ( +-  0.02% )

      3.091204859 seconds time elapsed                                          ( +-  0.72% )

And the following with the patch:

Performance counter stats for './bin/llvm-lit -v --no-execute projects/libcxx/test/libcxx/selftest/dsl/' (10 runs):

      2833.725264      task-clock (msec)         #    0.968 CPUs utilized            ( +-  0.62% )
              503      context-switches          #    0.178 K/sec                    ( +-  0.42% )
               29      cpu-migrations            #    0.010 K/sec                    ( +- 12.05% )
          208,416      page-faults               #    0.074 M/sec                    ( +-  0.01% )
    5,135,758,720      cycles                    #    1.812 GHz                      ( +-  0.20% )
    5,564,092,379      instructions              #    1.08  insn per cycle           ( +-  0.01% )
    1,112,432,802      branches                  #  392.569 M/sec                    ( +-  0.01% )
       29,604,154      branch-misses             #    2.66% of all branches          ( +-  0.04% )

      2.928603087 seconds time elapsed                                          ( +-  0.70% )

So almost 200ms here. The difference is probably bigger because the first measurements had my locale detection changes applied and this one didn't.

[ldionne]

Thanks a lot for the perf numbers and due diligence.

However, in most contexts, we run the test suite on at least a few tests, where a ~200ms speed up is literally unobservable because the tests are significantly slower than that anyway. I believe this isn't worth the increased complexity. Do you disagree? Do you have a use case where this speed up is observable?

[arichardson]

In D89110#2327483, @ldionne wrote:

Thanks a lot for the perf numbers and due diligence.

However, in most contexts, we run the test suite on at least a few tests, where a ~200ms speed up is literally unobservable because the tests are significantly slower than that anyway. I believe this isn't worth the increased complexity. Do you disagree? Do you have a use case where this speed up is observable?

The reason I noticed is that I have a shortcut in setup in my IDE to run a single test so I noticed that the lit startup time was very high. I agree that 200ms is not that much (and doesn't matter at all for the full testsuite), but it is a noticeable delay when running single tests.

[arichardson]

And obviously the big win here is the lower-level caching of feature checks, this change is just a small (but noticeable) improvement. It made much more of a difference before caching. Since I run most tests on QEMU emulating CHERI, doing checks via SSH takes a lot longer than on the local machine.

[ldionne]

I would rather keep the test harness code as straightforward as possible unless there is a sizeable performance benefit, which doesn't seem to be the case anymore since we do lower-level caching. So I would rather not move forward with this patch, if you think that's a reasonable argument.

[ldionne]

Note that I think this is superseded by the refactor in https://reviews.llvm.org/D90429. That refactor will also allow defining straight flags in the DSL, without any feature being tied to them.

[arichardson]

Thanks! D90429 does indeed look much better.

libcxx/utils/libcxx/test/dsl.py

class Feature(object):
   def getName(self, config):
     """
     Return the name of the feature.
 
     It is an error to call `f.getName(cfg)` if the feature `f` is not supported.
     """
     assert self.isSupported(config), \
       "Trying to get the name of a feature that is not supported in the given configuration"
+    return self._getNameUnchecked(config)
+
+  def _getNameUnchecked(self, config):
     name = self._name(config) if callable(self._name) else self._name
     if not isinstance(name, str):
       raise ValueError("Feature did not resolve to a name that's a string, got {}".format(name))
     return name
 
   def enableIn(self, config):
     """
     Enable a feature in a TestingConfig.
 
     The name of the feature is added to the set of available features of
     `config`, and any compile or link flags provided upon construction of
     the Feature are added to the end of the corresponding substitution in
     the config.
 
     It is an error to call `f.enableIn(cfg)` if the feature `f` is not
     supported in that TestingConfig (i.e. if `not f.isSupported(cfg)`).
     """
     assert self.isSupported(config), \
       "Trying to enable feature {} that is not supported in the given configuration".format(self._name)
+    self._enableInUnchecked(config)
 
+  def _enableInUnchecked(self, config):
     addTo = lambda subs, sub, flag: [(s, x + ' ' + flag) if s == sub else (s, x) for (s, x) in subs]
     if self._compileFlag:
       compileFlag = self._compileFlag(config) if callable(self._compileFlag) else self._compileFlag
       config.substitutions = addTo(config.substitutions, '%{compile_flags}', compileFlag)
     if self._linkFlag:
       linkFlag = self._linkFlag(config) if callable(self._linkFlag) else self._linkFlag
       config.substitutions = addTo(config.substitutions, '%{link_flags}', linkFlag)
-    config.available_features.add(self.getName(config))
+    config.available_features.add(self._getNameUnchecked(config))
+
+  def tryEnableIn(self, config):
+    """
+    Try to enable a feature in a TestingConfig (if it is supported).
 
+    This is equivalent to `if isSupported(config): enabledIn(config)`, but
+    is slightly faster since it can omit redundant checks.
+    """
+    if self.isSupported(config):
+      self._enableInUnchecked(config)
+      return True
+    return False
 
 def _str_to_bool(s):
   """
   Convert a string value to a boolean.
 
   True values are "y", "yes", "t", "true", "on" and "1", regardless of capitalization.
   False values are "n", "no", "f", "false", "off" and "0", regardless of capitalization.
   """

libcxx/utils/libcxx/test/newconfig.py

for param in parameters:
     feature = param.getFeature(config, lit_config.params)
     if feature:
       feature.enableIn(config)
       lit_config.note("Enabling Lit feature '{}' as a result of parameter '{}'".format(feature.getName(config), param.name))
 
   # Then, apply the automatically-detected features.
   printFeatures = []
   for feature in features:
-    if feature.isSupported(config):
-      feature.enableIn(config)
-      printFeatures.append(feature.getName(config))
+    if feature.tryEnableIn(config):
+      # We can avoid a potentially expensive assertion and use
+      # _getNameUnchecked since we know that the feature is enabled.
+      printFeatures.append(feature._getNameUnchecked(config))
   printFeatures = ["'{}'".format(f) for f in sorted(printFeatures)]
   lit_config.note("Enabling implicitly detected Lit features {}".format(', '.join(printFeatures)))
 
   # Print the basic substitutions
   for sub in ('%{cxx}', '%{flags}', '%{compile_flags}', '%{link_flags}', '%{exec}'):
     lit_config.note("Using {} substitution: '{}'".format(sub, _getSubstitution(sub, config)))