Bug 1191575 - Optimize the RecursiveMakeBackend._check_blacklisted_variables test
authorMike Hommey <mh+mozilla@glandium.org>
Thu, 06 Aug 2015 11:55:28 +0900
changeset 523048 de175d7e8eb26fddb0087bcc202bed58dd17f33d
parent 523047 33508b5aed65400a2ee36f76d984c111b33bfdf2
child 523049 21829aaf72e1ae094c6de7a35f3dcfaeb85ce5b8
push id81997
push usermh@glandium.org
push dateThu, 06 Aug 2015 05:44:27 +0000
treeherdertry@21829aaf72e1 [default view] [failures only]
bugs1191575, 1153566
milestone42.0a1
Bug 1191575 - Optimize the RecursiveMakeBackend._check_blacklisted_variables test Bug 1153566 changed the regexp used in that method in such a way that there has been a big hit in the time spent executing the make backend. On my machine, with the current code, mach build-backend indicates: Backend executed in 5.01s With the change from bug 1153566 reverted: Backend executed in 2.97s That's a significant regression for a 4-character change. But we can actually avoid using regexp in most cases, which can make things faster than they were. With this change, we get down to: Backend executed in 2.28s For reference, making the _check_blacklisted_variables method do nothing at all ends with: Backend executed in 2.20s
python/mozbuild/mozbuild/backend/recursivemake.py
--- a/python/mozbuild/mozbuild/backend/recursivemake.py
+++ b/python/mozbuild/mozbuild/backend/recursivemake.py
@@ -9,16 +9,17 @@ import os
 import re
 import types
 
 from collections import (
     defaultdict,
     namedtuple,
 )
 from StringIO import StringIO
+from itertools import chain
 
 from reftest import ReftestManifest
 
 from mozpack.copier import FilePurger
 from mozpack.manifests import (
     InstallManifest,
 )
 import mozpack.path as mozpath
@@ -64,90 +65,98 @@ from ..frontend.data import (
 )
 from ..util import (
     ensureParentDir,
     FileAvoidWrite,
 )
 from ..makeutil import Makefile
 
 MOZBUILD_VARIABLES = [
-    'ANDROID_GENERATED_RESFILES',
-    'ANDROID_RES_DIRS',
-    'ASFLAGS',
-    'CMSRCS',
-    'CMMSRCS',
-    'CPP_UNIT_TESTS',
-    'DIRS',
-    'DIST_INSTALL',
-    'EXTRA_DSO_LDOPTS',
-    'EXTRA_JS_MODULES',
-    'EXTRA_PP_COMPONENTS',
-    'EXTRA_PP_JS_MODULES',
-    'FORCE_SHARED_LIB',
-    'FORCE_STATIC_LIB',
-    'FINAL_LIBRARY',
-    'HOST_CSRCS',
-    'HOST_CMMSRCS',
-    'HOST_EXTRA_LIBS',
-    'HOST_LIBRARY_NAME',
-    'HOST_PROGRAM',
-    'HOST_SIMPLE_PROGRAMS',
-    'IS_COMPONENT',
-    'JAR_MANIFEST',
-    'JAVA_JAR_TARGETS',
-    'LD_VERSION_SCRIPT',
-    'LIBRARY_NAME',
-    'LIBS',
-    'MAKE_FRAMEWORK',
-    'MODULE',
-    'NO_DIST_INSTALL',
-    'NO_EXPAND_LIBS',
-    'NO_INTERFACES_MANIFEST',
-    'NO_JS_MANIFEST',
-    'OS_LIBS',
-    'PARALLEL_DIRS',
-    'PREF_JS_EXPORTS',
-    'PROGRAM',
-    'PYTHON_UNIT_TESTS',
-    'RESOURCE_FILES',
-    'SDK_HEADERS',
-    'SDK_LIBRARY',
-    'SHARED_LIBRARY_LIBS',
-    'SHARED_LIBRARY_NAME',
-    'SIMPLE_PROGRAMS',
-    'SONAME',
-    'STATIC_LIBRARY_NAME',
-    'TEST_DIRS',
-    'TOOL_DIRS',
+    b'ANDROID_GENERATED_RESFILES',
+    b'ANDROID_RES_DIRS',
+    b'ASFLAGS',
+    b'CMSRCS',
+    b'CMMSRCS',
+    b'CPP_UNIT_TESTS',
+    b'DIRS',
+    b'DIST_INSTALL',
+    b'EXTRA_DSO_LDOPTS',
+    b'EXTRA_JS_MODULES',
+    b'EXTRA_PP_COMPONENTS',
+    b'EXTRA_PP_JS_MODULES',
+    b'FORCE_SHARED_LIB',
+    b'FORCE_STATIC_LIB',
+    b'FINAL_LIBRARY',
+    b'HOST_CSRCS',
+    b'HOST_CMMSRCS',
+    b'HOST_EXTRA_LIBS',
+    b'HOST_LIBRARY_NAME',
+    b'HOST_PROGRAM',
+    b'HOST_SIMPLE_PROGRAMS',
+    b'IS_COMPONENT',
+    b'JAR_MANIFEST',
+    b'JAVA_JAR_TARGETS',
+    b'LD_VERSION_SCRIPT',
+    b'LIBRARY_NAME',
+    b'LIBS',
+    b'MAKE_FRAMEWORK',
+    b'MODULE',
+    b'NO_DIST_INSTALL',
+    b'NO_EXPAND_LIBS',
+    b'NO_INTERFACES_MANIFEST',
+    b'NO_JS_MANIFEST',
+    b'OS_LIBS',
+    b'PARALLEL_DIRS',
+    b'PREF_JS_EXPORTS',
+    b'PROGRAM',
+    b'PYTHON_UNIT_TESTS',
+    b'RESOURCE_FILES',
+    b'SDK_HEADERS',
+    b'SDK_LIBRARY',
+    b'SHARED_LIBRARY_LIBS',
+    b'SHARED_LIBRARY_NAME',
+    b'SIMPLE_PROGRAMS',
+    b'SONAME',
+    b'STATIC_LIBRARY_NAME',
+    b'TEST_DIRS',
+    b'TOOL_DIRS',
     # XXX config/Makefile.in specifies this in a make invocation
     #'USE_EXTENSION_MANIFEST',
-    'XPCSHELL_TESTS',
-    'XPIDL_MODULE',
+    b'XPCSHELL_TESTS',
+    b'XPIDL_MODULE',
 ]
 
 DEPRECATED_VARIABLES = [
-    'ANDROID_RESFILES',
-    'EXPORT_LIBRARY',
-    'EXTRA_LIBS',
-    'HOST_LIBS',
-    'LIBXUL_LIBRARY',
-    'MOCHITEST_A11Y_FILES',
-    'MOCHITEST_BROWSER_FILES',
-    'MOCHITEST_BROWSER_FILES_PARTS',
-    'MOCHITEST_CHROME_FILES',
-    'MOCHITEST_FILES',
-    'MOCHITEST_FILES_PARTS',
-    'MOCHITEST_METRO_FILES',
-    'MOCHITEST_ROBOCOP_FILES',
-    'MOZ_CHROME_FILE_FORMAT',
-    'SHORT_LIBNAME',
-    'TESTING_JS_MODULES',
-    'TESTING_JS_MODULE_DIR',
+    b'ANDROID_RESFILES',
+    b'EXPORT_LIBRARY',
+    b'EXTRA_LIBS',
+    b'HOST_LIBS',
+    b'LIBXUL_LIBRARY',
+    b'MOCHITEST_A11Y_FILES',
+    b'MOCHITEST_BROWSER_FILES',
+    b'MOCHITEST_BROWSER_FILES_PARTS',
+    b'MOCHITEST_CHROME_FILES',
+    b'MOCHITEST_FILES',
+    b'MOCHITEST_FILES_PARTS',
+    b'MOCHITEST_METRO_FILES',
+    b'MOCHITEST_ROBOCOP_FILES',
+    b'MOZ_CHROME_FILE_FORMAT',
+    b'SHORT_LIBNAME',
+    b'TESTING_JS_MODULES',
+    b'TESTING_JS_MODULE_DIR',
 ]
 
+MOZBUILD_VARIABLES_MESSAGE = 'It should only be defined in moz.build files.'
+
+DEPRECATED_VARIABLES_MESSAGE = (
+    'This variable has been deprecated. It does nothing. It must be removed '
+    'in order to build.'
+)
+
+
 class BackendMakeFile(object):
     """Represents a generated backend.mk file.
 
     This is both a wrapper around a file handle as well as a container that
     holds accumulated state.
 
     It's worth taking a moment to explain the make dependencies. The
     generated backend.mk as well as the Makefile.in (if it exists) are in the
@@ -723,26 +732,35 @@ class RecursiveMakeBackend(CommonBackend
             rule = makefile.create_rule(['$(all_absolute_unified_files)'])
             rule.add_dependencies(['$(CURDIR)/%: %'])
 
     def _check_blacklisted_variables(self, makefile_in, makefile_content):
         if b'EXTERNALLY_MANAGED_MAKE_FILE' in makefile_content:
             # Bypass the variable restrictions for externally managed makefiles.
             return
 
-        for x in MOZBUILD_VARIABLES:
-            if re.search(r'^[^#]*\b%s\s*[:?+]?=' % x, makefile_content, re.M):
-                raise Exception('Variable %s is defined in %s. It should '
-                    'only be defined in moz.build files.' % (x, makefile_in))
+        for l in makefile_content.splitlines():
+            l = l.strip()
+            # Don't check comments
+            if l.startswith(b'#'):
+                continue
+            for x in chain(MOZBUILD_VARIABLES, DEPRECATED_VARIABLES):
+                if x not in l:
+                    continue
 
-        for x in DEPRECATED_VARIABLES:
-            if re.search(r'^[^#]*\b%s\s*[:?+]?=' % x, makefile_content, re.M):
-                raise Exception('Variable %s is defined in %s. This variable '
-                    'has been deprecated. It does nothing. It must be removed '
-                    'in order to build.' % (x, makefile_in))
+                # Finding the variable name in the Makefile is not enough: it
+                # may just appear as part of something else, like DIRS appears
+                # in GENERATED_DIRS.
+                if re.search(r'\b%s\s*[:?+]?=' % x, l, re.M):
+                    if x in MOZBUILD_VARIABLES:
+                        message = MOZBUILD_VARIABLES_MESSAGE
+                    else:
+                        message = DEPRECATED_VARIABLES_MESSAGE
+                    raise Exception('Variable %s is defined in %s. %s'
+                                    % (x, makefile_in, message))
 
     def consume_finished(self):
         CommonBackend.consume_finished(self)
 
         for objdir, backend_file in sorted(self._backend_files.items()):
             srcdir = backend_file.srcdir
             with self._write_file(fh=backend_file) as bf:
                 makefile_in = mozpath.join(srcdir, 'Makefile.in')