From 3e9de33fa693a14e05143346646b2f87993cca4d Mon Sep 17 00:00:00 2001 From: William Deegan Date: Sun, 4 Dec 2022 14:02:45 -0800 Subject: Added --experimental=tm_v2 which switches to use Andrew Morrow's new ParallelJob implementation. WHich should scale much better for highly parallel builds --- CHANGES.txt | 2 ++ RELEASE.txt | 3 +++ SCons/Script/SConsOptions.py | 4 ++-- SCons/Taskmaster/Job.py | 7 ++++--- doc/man/scons.xml | 5 +++-- test/option/taskmastertrace.py | 3 +-- 6 files changed, 15 insertions(+), 9 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 5e7f6d4..8818eed 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -32,6 +32,8 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER - Moved rpm and debian directories under packaging - Added logic to help packagers enable reproducible builds into packaging/etc/. Please read packaging/etc/README.txt if you are interested. + - Added --experimental=tm_v2, which enables Andrew Morrow's new NewParallel Job implementation. + This should scale much better for highly parallel builds. From Dan Mezhiborsky: - Add newline to end of compilation db (compile_commands.json). diff --git a/RELEASE.txt b/RELEASE.txt index 801062a..c7e7c4d 100644 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -24,6 +24,9 @@ NEW FUNCTIONALITY - Added ValidateOptions() which will check that all command line options are in either those specified by SCons itself, or by AddOption() in SConstruct/SConscript. It should not be called until all AddOption() calls are completed. Resolves Issue #4187 +- Added --experimental=tm_v2, which enables Andrew Morrow's NewParallel Job implementation. + This should scale much better for highly parallel builds. + DEPRECATED FUNCTIONALITY ------------------------ diff --git a/SCons/Script/SConsOptions.py b/SCons/Script/SConsOptions.py index f3b4708..a3e3ea8 100644 --- a/SCons/Script/SConsOptions.py +++ b/SCons/Script/SConsOptions.py @@ -40,7 +40,7 @@ SUPPRESS_HELP = optparse.SUPPRESS_HELP diskcheck_all = SCons.Node.FS.diskcheck_types() -experimental_features = {'warp_speed', 'transporter', 'ninja'} +experimental_features = {'warp_speed', 'transporter', 'ninja', 'tm_v2'} def diskcheck_convert(value): @@ -65,7 +65,7 @@ def diskcheck_convert(value): class SConsValues(optparse.Values): """ Holder class for uniform access to SCons options, regardless - of whether or not they can be set on the command line or in the + of whether they can be set on the command line or in the SConscript files (using the SetOption() function). A SCons option value can originate three different ways: diff --git a/SCons/Taskmaster/Job.py b/SCons/Taskmaster/Job.py index 853fe63..ef10df5 100644 --- a/SCons/Taskmaster/Job.py +++ b/SCons/Taskmaster/Job.py @@ -40,6 +40,7 @@ from enum import Enum import SCons.Errors import SCons.Warnings + # The default stack size (in kilobytes) of the threads used to execute # jobs in parallel. # @@ -53,8 +54,6 @@ default_stack_size = 256 interrupt_msg = 'Build interrupted.' -USE_NEW_PARALLEL = os.environ.get('SCONS_NEW_PARALLEL', False) - class InterruptState: def __init__(self): self.interrupted = False @@ -84,6 +83,7 @@ class Jobs: class can't do it, it gets reset to 1. Wrapping interfaces that care should check the value of 'num_jobs' after initialization. """ + from SCons.Script import GetOption self.job = None if num > 1: @@ -92,10 +92,11 @@ class Jobs: stack_size = default_stack_size try: - if USE_NEW_PARALLEL: + if 'tm_v2' in GetOption('experimental'): self.job = NewParallel(taskmaster, num, stack_size) else: self.job = LegacyParallel(taskmaster, num, stack_size) + self.num_jobs = num except NameError: pass diff --git a/doc/man/scons.xml b/doc/man/scons.xml index 0278d87..4f1987d 100644 --- a/doc/man/scons.xml +++ b/doc/man/scons.xml @@ -1125,11 +1125,12 @@ the mechanisms in the specified order. the special tokens all or none. A comma-separated string can be used to select multiple features. The default setting is none. - Current available features are: ninja. + Current available features are: ninja, tm_v2. No Support offered for any features or tools enabled by this flag. - Available since &scons; 4.2. + ninjaAvailable since &scons; 4.2. + tm_v2Available since &scons; 4.4.1 diff --git a/test/option/taskmastertrace.py b/test/option/taskmastertrace.py index 0e44896..f30d725 100644 --- a/test/option/taskmastertrace.py +++ b/test/option/taskmastertrace.py @@ -55,8 +55,7 @@ test.run(arguments='--taskmastertrace=trace.out .', stdout=expect_stdout) test.must_match_file('trace.out', 'taskmaster_expected_file_1.txt', mode='r') # Test NewParallel Job implementation -os.environ['SCONS_NEW_PARALLEL'] = "1" -test.run(arguments='-j 2 --taskmastertrace=new_parallel_trace.out .') +test.run(arguments='-j 2 --experimental=tm_v2 --taskmastertrace=new_parallel_trace.out .') new_trace = test.read('new_parallel_trace.out', mode='r') thread_id = re.compile(r'\[Thread:\d+\]') -- cgit v0.12 From 67c311bd014889882fe76e4b25230b6386f07adf Mon Sep 17 00:00:00 2001 From: William Deegan Date: Sun, 4 Dec 2022 14:19:08 -0800 Subject: Fix broken tests --- test/option/fixture/SConstruct__taskmastertrace | 9 +++++++++ test/option/fixture/SConstruct__taskmastertrace.py | 9 --------- test/option/option--experimental.py | 7 ++++--- test/option/taskmastertrace.py | 2 +- 4 files changed, 14 insertions(+), 13 deletions(-) create mode 100644 test/option/fixture/SConstruct__taskmastertrace delete mode 100644 test/option/fixture/SConstruct__taskmastertrace.py diff --git a/test/option/fixture/SConstruct__taskmastertrace b/test/option/fixture/SConstruct__taskmastertrace new file mode 100644 index 0000000..91e8c91 --- /dev/null +++ b/test/option/fixture/SConstruct__taskmastertrace @@ -0,0 +1,9 @@ +DefaultEnvironment(tools=[]) +env = Environment(tools=[]) + +# We name the files 'Tfile' so that they will sort after the SConstruct +# file regardless of whether the test is being run on a case-sensitive +# or case-insensitive system. + +env.Command('Tfile.out', 'Tfile.mid', Copy('$TARGET', '$SOURCE')) +env.Command('Tfile.mid', 'Tfile.in', Copy('$TARGET', '$SOURCE')) diff --git a/test/option/fixture/SConstruct__taskmastertrace.py b/test/option/fixture/SConstruct__taskmastertrace.py deleted file mode 100644 index 91e8c91..0000000 --- a/test/option/fixture/SConstruct__taskmastertrace.py +++ /dev/null @@ -1,9 +0,0 @@ -DefaultEnvironment(tools=[]) -env = Environment(tools=[]) - -# We name the files 'Tfile' so that they will sort after the SConstruct -# file regardless of whether the test is being run on a case-sensitive -# or case-insensitive system. - -env.Command('Tfile.out', 'Tfile.mid', Copy('$TARGET', '$SOURCE')) -env.Command('Tfile.mid', 'Tfile.in', Copy('$TARGET', '$SOURCE')) diff --git a/test/option/option--experimental.py b/test/option/option--experimental.py index 51f3546..324de99 100644 --- a/test/option/option--experimental.py +++ b/test/option/option--experimental.py @@ -36,12 +36,13 @@ test.file_fixture('fixture/SConstruct__experimental', 'SConstruct') tests = [ ('.', []), ('--experimental=ninja', ['ninja']), - ('--experimental=all', ['ninja', 'transporter', 'warp_speed']), + ('--experimental=tm_v2', ['tm_v2']), + ('--experimental=all', ['ninja', 'tm_v2', 'transporter', 'warp_speed']), ('--experimental=none', []), ] for args, exper in tests: - read_string = """All Features=ninja,transporter,warp_speed + read_string = """All Features=ninja,tm_v2,transporter,warp_speed Experimental=%s """ % (exper) test.run(arguments=args, @@ -50,7 +51,7 @@ Experimental=%s test.run(arguments='--experimental=warp_drive', stderr="""usage: scons [OPTIONS] [VARIABLES] [TARGETS] -SCons Error: option --experimental: invalid choice: 'warp_drive' (choose from 'all','none','ninja','transporter','warp_speed') +SCons Error: option --experimental: invalid choice: 'warp_drive' (choose from 'all','none','ninja','tm_v2','transporter','warp_speed') """, status=2) diff --git a/test/option/taskmastertrace.py b/test/option/taskmastertrace.py index f30d725..99de718 100644 --- a/test/option/taskmastertrace.py +++ b/test/option/taskmastertrace.py @@ -33,7 +33,7 @@ import TestSCons test = TestSCons.TestSCons() -test.file_fixture('fixture/SConstruct__taskmastertrace.py', 'SConstruct') +test.file_fixture('fixture/SConstruct__taskmastertrace', 'SConstruct') test.file_fixture('fixture/taskmaster_expected_stdout_1.txt', 'taskmaster_expected_stdout_1.txt') test.file_fixture('fixture/taskmaster_expected_file_1.txt', 'taskmaster_expected_file_1.txt') test.file_fixture('fixture/taskmaster_expected_new_parallel.txt', 'taskmaster_expected_new_parallel.txt') -- cgit v0.12 From c30dbeeb1b0426e934dce71e61edfe2c6b8afcc3 Mon Sep 17 00:00:00 2001 From: William Deegan Date: Sun, 4 Dec 2022 14:59:53 -0800 Subject: modernize JobTests to just use unittest.main(), fix JobTests to have a reasonable value for OptionsParser.values.experimental, so Jobs doesn't crash when referencing it and getting a None instead of an iterable --- SCons/Taskmaster/Job.py | 3 ++- SCons/Taskmaster/JobTests.py | 59 ++++++++++++++++++++++++-------------------- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/SCons/Taskmaster/Job.py b/SCons/Taskmaster/Job.py index ef10df5..81bf5e4 100644 --- a/SCons/Taskmaster/Job.py +++ b/SCons/Taskmaster/Job.py @@ -92,7 +92,8 @@ class Jobs: stack_size = default_stack_size try: - if 'tm_v2' in GetOption('experimental'): + experimental_option = GetOption('experimental') + if 'tm_v2' in experimental_option: self.job = NewParallel(taskmaster, num, stack_size) else: self.job = LegacyParallel(taskmaster, num, stack_size) diff --git a/SCons/Taskmaster/JobTests.py b/SCons/Taskmaster/JobTests.py index 57b548c..c9c3535 100644 --- a/SCons/Taskmaster/JobTests.py +++ b/SCons/Taskmaster/JobTests.py @@ -24,12 +24,10 @@ import unittest import random import math -import sys import os -import TestUnit - import SCons.Taskmaster.Job +from SCons.Script.Main import OptionsParser def get_cpu_nums(): @@ -244,10 +242,25 @@ class Taskmaster: def cleanup(self): pass + SaveThreadPool = None ThreadPoolCallList = [] -class ParallelTestCase(unittest.TestCase): + +class JobTestCase(unittest.TestCase): + """ + Setup common items needed for many Job test cases + """ + def setUp(self) -> None: + """ + Simulating real options parser experimental value. + Since we're in a unit test we're actually using FakeOptionParser() + Which has no values and no defaults. + """ + OptionsParser.values.experimental = [] + + +class ParallelTestCase(JobTestCase): def runTest(self): """test parallel jobs""" @@ -334,7 +347,9 @@ class SerialTestCase(unittest.TestCase): self.assertFalse(taskmaster.num_failed, "some task(s) failed to execute") -class NoParallelTestCase(unittest.TestCase): + +class NoParallelTestCase(JobTestCase): + def runTest(self): """test handling lack of parallel support""" def NoParallel(tm, num, stack_size): @@ -378,7 +393,9 @@ class SerialExceptionTestCase(unittest.TestCase): self.assertTrue(taskmaster.num_postprocessed == 1, "exactly one task should have been postprocessed") -class ParallelExceptionTestCase(unittest.TestCase): + +class ParallelExceptionTestCase(JobTestCase): + def runTest(self): """test parallel jobs with tasks that raise exceptions""" @@ -447,7 +464,8 @@ class badpreparenode (badnode): def prepare(self): raise Exception('badpreparenode exception') -class _SConsTaskTest(unittest.TestCase): + +class _SConsTaskTest(JobTestCase): def _test_seq(self, num_jobs): for node_seq in [ @@ -541,31 +559,18 @@ class ParallelTaskTest(_SConsTaskTest): """test parallel jobs with actual Taskmaster and Task""" self._test_seq(num_jobs) + OptionsParser.values.experimental=['tm_v2'] + self._test_seq(num_jobs) -#--------------------------------------------------------------------- -def suite(): - suite = unittest.TestSuite() - suite.addTest(ParallelTestCase()) - suite.addTest(SerialTestCase()) - suite.addTest(NoParallelTestCase()) - suite.addTest(SerialExceptionTestCase()) - suite.addTest(ParallelExceptionTestCase()) - suite.addTest(SerialTaskTest()) - suite.addTest(ParallelTaskTest()) - return suite + +#--------------------------------------------------------------------- if __name__ == "__main__": - runner = TestUnit.cli.get_runner() - result = runner().run(suite()) - if (len(result.failures) == 0 - and len(result.errors) == 1 - and isinstance(result.errors[0][0], SerialTestCase) - and isinstance(result.errors[0][1][0], NoThreadsException)): - sys.exit(2) - elif not result.wasSuccessful(): - sys.exit(1) + unittest.main() + + # Local Variables: # tab-width:4 -- cgit v0.12 From 165ba8f56e5f27fb63ff400b689b072d09052ed9 Mon Sep 17 00:00:00 2001 From: William Deegan Date: Sun, 4 Dec 2022 15:20:50 -0800 Subject: [ci skip] Fix formatting for which versions introduced which experimental features --- SCons/Taskmaster/JobTests.py | 1 + doc/man/scons.xml | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/SCons/Taskmaster/JobTests.py b/SCons/Taskmaster/JobTests.py index c9c3535..9e7b080 100644 --- a/SCons/Taskmaster/JobTests.py +++ b/SCons/Taskmaster/JobTests.py @@ -559,6 +559,7 @@ class ParallelTaskTest(_SConsTaskTest): """test parallel jobs with actual Taskmaster and Task""" self._test_seq(num_jobs) + # Now run test with NewParallel() instead of LegacyParallel OptionsParser.values.experimental=['tm_v2'] self._test_seq(num_jobs) diff --git a/doc/man/scons.xml b/doc/man/scons.xml index 4f1987d..2bdf2b9 100644 --- a/doc/man/scons.xml +++ b/doc/man/scons.xml @@ -1129,8 +1129,8 @@ the mechanisms in the specified order. No Support offered for any features or tools enabled by this flag. - ninjaAvailable since &scons; 4.2. - tm_v2Available since &scons; 4.4.1 + ninja Available since &scons; 4.2. + tm_v2 Available since &scons; 4.4.1 -- cgit v0.12 From cce148bf826d3972532a39ea29d062dfc436b05c Mon Sep 17 00:00:00 2001 From: William Deegan Date: Sun, 4 Dec 2022 15:26:08 -0800 Subject: [ci skip] Add note to RELEASE/CHANGE that tm_v2 can be set via SetOption() --- CHANGES.txt | 2 +- RELEASE.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 8818eed..f2a42e2 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -33,7 +33,7 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER - Added logic to help packagers enable reproducible builds into packaging/etc/. Please read packaging/etc/README.txt if you are interested. - Added --experimental=tm_v2, which enables Andrew Morrow's new NewParallel Job implementation. - This should scale much better for highly parallel builds. + This should scale much better for highly parallel builds. You can also enable this via SetOption(). From Dan Mezhiborsky: - Add newline to end of compilation db (compile_commands.json). diff --git a/RELEASE.txt b/RELEASE.txt index c7e7c4d..2a32831 100644 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -25,7 +25,7 @@ NEW FUNCTIONALITY those specified by SCons itself, or by AddOption() in SConstruct/SConscript. It should not be called until all AddOption() calls are completed. Resolves Issue #4187 - Added --experimental=tm_v2, which enables Andrew Morrow's NewParallel Job implementation. - This should scale much better for highly parallel builds. + This should scale much better for highly parallel builds. You can also enable this via SetOption(). DEPRECATED FUNCTIONALITY -- cgit v0.12 From df0fb6b43ab70e1b0b787aba4d3853ee1c5e6db1 Mon Sep 17 00:00:00 2001 From: William Deegan Date: Mon, 5 Dec 2022 09:36:16 -0800 Subject: [ci skip] Add pylint annotation and comment regarding importing GetOption in a method instead of at top of file. (Thanks mwichmann for providing feedback and pylint syntax) --- SCons/Taskmaster/Job.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/SCons/Taskmaster/Job.py b/SCons/Taskmaster/Job.py index 81bf5e4..a63b529 100644 --- a/SCons/Taskmaster/Job.py +++ b/SCons/Taskmaster/Job.py @@ -83,6 +83,10 @@ class Jobs: class can't do it, it gets reset to 1. Wrapping interfaces that care should check the value of 'num_jobs' after initialization. """ + + # Importing GetOption here instead of at top of file to avoid + # circular imports + # pylint: disable=import-outside-toplevel from SCons.Script import GetOption self.job = None -- cgit v0.12