Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1915303002:
[tools] Specify affinity and raise priority when benchmarking (Closed)
|
Created:
4 years, 8 months ago by Mircea Trofin Modified:
4 years, 7 months ago Reviewers:
Michael Achenbach CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[tools] Specify affinity and raise priority when benchmarking
I plan to drive these options from the infra recipe.
Raising the priority of the benchmarked process, and fixing
its affinity to some other CPU than 0 improves stability of
measurements.
BUG=
Committed: https://crrev.com/5b519033f0332efc8f2031cd4a3388fd12c5bad9
Cr-Commit-Position: refs/heads/master@{#35912}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 20
Patch Set 3 : #
Total comments: 3
Patch Set 4 : #Patch Set 5 : #Messages
Total messages: 28 (10 generated)
Description was changed from ========== [tools] Options to specify affinity and raise priority when benchmarking BUG= ========== to ========== [tools] Specify affinity and raise priority when benchmarking I plan to drive these options from the infra recipe. Raising the priority of the benchmarked process, and fixing its affinity to some other CPU than 0 improves stability of measurements. BUG= ==========
mtrofin@google.com changed reviewers: + machenbach@google.com
machenbach@chromium.org changed reviewers: + machenbach@chromium.org - machenbach@google.com
Changed to chromium.org reviewer address...
got some questions https://codereview.chromium.org/1915303002/diff/1/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/1/tools/run_perf.py#newcode619 tools/run_perf.py:619: self.command_prefix += ["-p", "-20"] My documentation says: -p STATIC_PRIORITY usually 1-99; only for FIFO or RR higher numbers means higher priority https://codereview.chromium.org/1915303002/diff/1/tools/run_perf.py#newcode626 tools/run_perf.py:626: self.command_prefix += ["-a", str(core)] So if options.affinitize == 2, this is set to "-a 3". Is that what the tool wants or a binary bit pattern like 011? I also wonder, setting several bits means several cores right? The description below says "_the_ specified core" or did I get something wrong? Which one is core 0 exactly? 011 ^ or 011 ^ According to the CL description, core 0 should be avoided. Is it?
One more question: should we rather require the whole run_tools.py be run as sudo, rather than sudo-ing inside? Especially for the next CL, where we enable/disable aslr and slow down the CPU, it'd simplify the code to sudo the whole run_tools.py. This is because doing these (aslr/speed) consists of reading/writing to a few device files. Writing requires sudo. From what I can see, there's no way to request sudo at the time of opening a file. The alternative is to sudo for writing only, which complicates the code somewhat - different ways of doing related things. https://codereview.chromium.org/1915303002/diff/1/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/1/tools/run_perf.py#newcode619 tools/run_perf.py:619: self.command_prefix += ["-p", "-20"] On 2016/04/27 14:55:00, Michael Achenbach wrote: > My documentation says: > -p STATIC_PRIORITY usually 1-99; only for FIFO or RR > higher numbers means higher priority Ugh. should be -n, not -p. Thanks. https://codereview.chromium.org/1915303002/diff/1/tools/run_perf.py#newcode626 tools/run_perf.py:626: self.command_prefix += ["-a", str(core)] On 2016/04/27 14:55:00, Michael Achenbach wrote: > So if options.affinitize == 2, this is set to "-a 3". Is that what the tool > wants or a binary bit pattern like 011? > > I also wonder, setting several bits means several cores right? The description > below says "_the_ specified core" or did I get something wrong? > > Which one is core 0 exactly? > 011 > ^ > or > 011 > ^ > > According to the CL description, core 0 should be avoided. Is it? Oh, that's a bug, indeed. It should be core = if options.affinitize == 0: 0 else (1 << (options.affinitize - 1) ) I'd still let developers specify core 0 if they so wish. I'll make the commit comment more clear that way.
https://codereview.chromium.org/1915303002/diff/1/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/1/tools/run_perf.py#newcode626 tools/run_perf.py:626: self.command_prefix += ["-a", str(core)] On 2016/04/27 15:56:35, Mircea Trofin wrote: > On 2016/04/27 14:55:00, Michael Achenbach wrote: > > So if options.affinitize == 2, this is set to "-a 3". Is that what the tool > > wants or a binary bit pattern like 011? > > > > I also wonder, setting several bits means several cores right? The description > > below says "_the_ specified core" or did I get something wrong? > > > > Which one is core 0 exactly? > > 011 > > ^ > > or > > 011 > > ^ > > > > According to the CL description, core 0 should be avoided. Is it? > > Oh, that's a bug, indeed. It should be core = if options.affinitize == 0: 0 > else (1 << (options.affinitize - 1) ) > > I'd still let developers specify core 0 if they so wish. I'll make the commit > comment more clear that way. Could you add in a comment an example of how this exactly looks like? E.g. for core number 3 and for 0 and what is expected to be passed to schedtool.
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Fixed the core issue, after re-reading the schedtool docs. Also added the options for noaslr and cpu governor. https://codereview.chromium.org/1915303002/diff/1/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/1/tools/run_perf.py#newcode626 tools/run_perf.py:626: self.command_prefix += ["-a", str(core)] On 2016/04/27 17:23:29, Michael Achenbach wrote: > On 2016/04/27 15:56:35, Mircea Trofin wrote: > > On 2016/04/27 14:55:00, Michael Achenbach wrote: > > > So if options.affinitize == 2, this is set to "-a 3". Is that what the tool > > > wants or a binary bit pattern like 011? > > > > > > I also wonder, setting several bits means several cores right? The > description > > > below says "_the_ specified core" or did I get something wrong? > > > > > > Which one is core 0 exactly? > > > 011 > > > ^ > > > or > > > 011 > > > ^ > > > > > > According to the CL description, core 0 should be avoided. Is it? > > > > Oh, that's a bug, indeed. It should be core = if options.affinitize == 0: 0 > > else (1 << (options.affinitize - 1) ) > > > > I'd still let developers specify core 0 if they so wish. I'll make the commit > > comment more clear that way. > > Could you add in a comment an example of how this exactly looks like? E.g. for > core number 3 and for 0 and what is expected to be passed to schedtool. Done.
https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:628: self.command_prefix += ["-a", ("0x%x" % core)] Another question to this: Now we set exactly one core. Does that sacrifice performance for v8 (as v8 is multi-threaded)? Would we not get equally stable results (without sacrificing speed) with a pattern that sets several cores but avoids core 0? Like 0xe
https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:628: self.command_prefix += ["-a", ("0x%x" % core)] On 2016/04/28 13:03:07, Michael Achenbach wrote: > Another question to this: Now we set exactly one core. Does that sacrifice > performance for v8 (as v8 is multi-threaded)? Would we not get equally stable > results (without sacrificing speed) with a pattern that sets several cores but > avoids core 0? Like 0xe Multi-core may introduce other sources of variance, for instance, if the cores are not sharing caches. For my goal - stable measurements for codegen - I may even need to avoid v8's multi-threading altogether.
https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:812: f = os.open("/proc/sys/kernel/randomize_va_space", os.O_RDONLY) Could you use with clauses? Also below. with open("/proc/sys/kernel/randomize_va_space") as f: return int(f.read().strip()) https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:827: if value != new_value: raise Exception("Present value is %s" % new_value) Maybe move this condition outside the try block to avoid catching rethrowing this exception. https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:836: [first, last] = map(int, indexes.split("-")) first, last = map(int, indexes.split("-")) should work https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:852: for cpu_index in cpu_indices: nit: Maybe inline GetCPUCoresRange() also in method below https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:859: else: elif ret != val: https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:877: if cur_value != value: same as above - move condition outside try https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:933: help="Set cpu governor to specified policy for the" nit: Space before " to not concatenate strings. https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:1042: Please refactor and put everything between setting/unsetting these things into a separate method. Then put it in a "try" clause and put the code below into "finally" to make sure we reset to original state also on other test runner errors. To make it easier, maybe move the reset code to the end of this method.
https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:812: f = os.open("/proc/sys/kernel/randomize_va_space", os.O_RDONLY) On 2016/04/28 14:47:44, Michael Achenbach wrote: > Could you use with clauses? Also below. > with open("/proc/sys/kernel/randomize_va_space") as f: > return int(f.read().strip()) Done. https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:827: if value != new_value: raise Exception("Present value is %s" % new_value) On 2016/04/28 14:47:44, Michael Achenbach wrote: > Maybe move this condition outside the try block to avoid catching rethrowing > this exception. Done. https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:836: [first, last] = map(int, indexes.split("-")) On 2016/04/28 14:47:44, Michael Achenbach wrote: > first, last = map(int, indexes.split("-")) > should work Done. https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:852: for cpu_index in cpu_indices: On 2016/04/28 14:47:44, Michael Achenbach wrote: > nit: Maybe inline GetCPUCoresRange() > also in method below I didn't understand what you meant here. https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:859: else: On 2016/04/28 14:47:44, Michael Achenbach wrote: > elif ret != val: Done. https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:877: if cur_value != value: On 2016/04/28 14:47:44, Michael Achenbach wrote: > same as above - move condition outside try Done. https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:933: help="Set cpu governor to specified policy for the" On 2016/04/28 14:47:43, Michael Achenbach wrote: > nit: Space before " to not concatenate strings. Done. https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:1042: On 2016/04/28 14:47:44, Michael Achenbach wrote: > Please refactor and put everything between setting/unsetting these things into a > separate method. Then put it in a "try" clause and put the code below into > "finally" to make sure we reset to original state also on other test runner > errors. > > To make it easier, maybe move the reset code to the end of this method. Thanks for the suggestion - this makes sense. I moved it all under a resource class.
lgtm with optional suggestions https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:852: for cpu_index in cpu_indices: On 2016/04/28 16:02:46, Mircea Trofin wrote: > On 2016/04/28 14:47:44, Michael Achenbach wrote: > > nit: Maybe inline GetCPUCoresRange() > > also in method below > > I didn't understand what you meant here. I meant to skip the local variable. But now that the name got longer it's only useful if you don't breach the 80 char limit: cpu_indices = CustomMachineConfiguration.GetCPUCoresRange() for cpu_index in cpu_indices: into for cpu_index in CustomMachineConfiguration.GetCPUCoresRange(): https://codereview.chromium.org/1915303002/diff/80001/tools/run_perf.py#newco... tools/run_perf.py:1042: On 2016/04/28 16:02:47, Mircea Trofin wrote: > On 2016/04/28 14:47:44, Michael Achenbach wrote: > > Please refactor and put everything between setting/unsetting these things into > a > > separate method. Then put it in a "try" clause and put the code below into > > "finally" to make sure we reset to original state also on other test runner > > errors. > > > > To make it easier, maybe move the reset code to the end of this method. > > Thanks for the suggestion - this makes sense. I moved it all under a resource > class. Very nice! https://codereview.chromium.org/1915303002/diff/100001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/100001/tools/run_perf.py#newc... tools/run_perf.py:815: I like this! https://codereview.chromium.org/1915303002/diff/100001/tools/run_perf.py#newc... tools/run_perf.py:831: def BackupASLR(self): The next 6 methods are each called only once and have one line of code. I think it might make sense to inline their code in the two methods above.
FYI: please check that the unittests still work (this code doesn't need any as it's tough to test this) cd tools/unittests python -m unittest run_perf_test
On 2016/04/29 07:42:17, Michael Achenbach wrote: > FYI: please check that the unittests still work (this code doesn't need any as > it's tough to test this) > > cd tools/unittests > python -m unittest run_perf_test FYI: To run the unittests python-coverage and python-mock are required.
On 2016/04/29 07:43:39, Michael Achenbach wrote: > On 2016/04/29 07:42:17, Michael Achenbach wrote: > > FYI: please check that the unittests still work (this code doesn't need any as > > it's tough to test this) > > > > cd tools/unittests > > python -m unittest run_perf_test > > FYI: To run the unittests python-coverage and python-mock are required. Checked - got the "OK" at the end of the run (and thanks for the dependency stuff, I didn't have them :))
https://codereview.chromium.org/1915303002/diff/100001/tools/run_perf.py File tools/run_perf.py (right): https://codereview.chromium.org/1915303002/diff/100001/tools/run_perf.py#newc... tools/run_perf.py:831: def BackupASLR(self): On 2016/04/29 07:40:32, Michael Achenbach wrote: > The next 6 methods are each called only once and have one line of code. I think > it might make sense to inline their code in the two methods above. Done.
The CQ bit was checked by mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1915303002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1915303002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915303002/140001
Message was sent while issue was closed.
Description was changed from ========== [tools] Specify affinity and raise priority when benchmarking I plan to drive these options from the infra recipe. Raising the priority of the benchmarked process, and fixing its affinity to some other CPU than 0 improves stability of measurements. BUG= ========== to ========== [tools] Specify affinity and raise priority when benchmarking I plan to drive these options from the infra recipe. Raising the priority of the benchmarked process, and fixing its affinity to some other CPU than 0 improves stability of measurements. BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [tools] Specify affinity and raise priority when benchmarking I plan to drive these options from the infra recipe. Raising the priority of the benchmarked process, and fixing its affinity to some other CPU than 0 improves stability of measurements. BUG= ========== to ========== [tools] Specify affinity and raise priority when benchmarking I plan to drive these options from the infra recipe. Raising the priority of the benchmarked process, and fixing its affinity to some other CPU than 0 improves stability of measurements. BUG= Committed: https://crrev.com/5b519033f0332efc8f2031cd4a3388fd12c5bad9 Cr-Commit-Position: refs/heads/master@{#35912} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5b519033f0332efc8f2031cd4a3388fd12c5bad9 Cr-Commit-Position: refs/heads/master@{#35912} |
