Plumb in extra instrumentation mode iterations runners flags. #367
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
If this is what we want, I'll also add a test for: |
|
Before I read the diff, @vext01 I'm pretty sure that we don't have any Travis tests with instrumentation. Would it be prohibitively slow to add one? |
|
We could add one for the legacy instrumentation interface (not using these flags). Is that what you mean? |
|
Yes, and then adapt it to the new API so we can check for problems. |
|
Perhaps eventually, but I don't want to port the legacy instrumentation over just yet, as I've got bigger fish to fry right now. |
|
Sure, I'm just concerned that we're going to have a bunch of PRs that incrementally change or add to the instrumentation framework and we're not testing it at all. |
snim2
reviewed
Jan 29, 2018
| # The new instrumentation switches are unused here, as the PyPy | ||
| # instrumentation code uses the legacy instrumentation framework. We | ||
| # still check the right number of args is passed. | ||
| if num_args != 8: |
There was a problem hiding this comment.
Maybe make 8 and 6 variables here?
There was a problem hiding this comment.
Ah, it's hardly worth it in this case. Do you feel strongly?
snim2
reviewed
Jan 29, 2018
| counting for until we find the first outstanding (O) record. | ||
|
|
||
| Instead, this method does a pass over the manifest searching for | ||
| records whose key is `self.next_exec_key`. |
There was a problem hiding this comment.
I like this very clear explanation!
There was a problem hiding this comment.
Thanks. It took a while to get the wording right on this one!
snim2
reviewed
Jan 29, 2018
krun/vm_defs.py
Outdated
| @@ -222,10 +222,16 @@ def _run_exec(self, args, heap_lim_k, stack_lim_k, bench_env_changes=None, | |||
| # Tack on the instrumentation flag | |||
| # All runners accept this flag, even if instrumentation is not | |||
| # implemented for the VM in question. | |||
| #print(key) | |||
|
@fsfod Is this interface sufficient for you? |
|
I'm in the process of adapting the my iterations_runner to test it it. I think it should be enough, |
|
OK, I'll wait to see the outcome before I carry on. |
|
@vext01 I guess i also needed the instrumentation data directory path and the directory needs to be created before the first benchmark is run. I also had permission issues with the iterations_runner writing to the instr_data directory. |
|
Can you show me the invocation and the error please? |
|
@vext01 Heres the start of the log with the 2 things i was talking about. Click to expand log text |
OK, I think I know what is happening here. Bear with me, I have a couple of other PRs to look at, then I'll be back onto this. |
|
The problem is in this code: Lines 441 to 457 in 30d9c6b Krun is running as a different user to the iterations runners, so the directory doesn't have the right permissions. We can either:
The former is simpler, so I think we should try this first. Any thoughts @fsfod ? |
|
The last commit shows how that would work. Also killed the debug code. |
|
I'm not really a fan of the copy method but at least the first two errors are fixed now. I still need to somehow get the instrumentation directory path in the runner. I can't keep it hard coded because its based on the krun file name |
|
Yes, I was thinking about his too. I recommenced we replace the |
|
[Also I need to fix the tests] |
|
That could work for me would it be an empty string otherwise? |
|
If we put all of the instrumentation flags on the end, we can assume the existence of more flags to mean that instrumentation mode is on. |
|
Try this. So the new switches format is like this: krun/iterations_runners/iterations_runner.py Lines 43 to 47 in da9cbed Makes sense? If it looks good, I'll implement the same for the other runners. |
|
yes, I have it all working correctly now with the latest changes, but krun is also still producing empty json files in the instrumentation directory |
|
Yeah, it will do for now. Once we port the PyPy and JVM instrumentation across(#359), we can kill that. |
|
I'm going to implement the new switch structure for the other runners today. |
|
This is almost ready to go, but I'd like to run it under "production" conditions, i.e. using the warmup experiment. I don't have a free Debian 8 machine to hand, so I may have to set up a VM or an old laptop or something. |
|
I may be able to test on Debian 9 without MSR support using the VMs in the apt repo... |
|
I'm going to test this in a debian 8 VM today. |
|
And finally tested. Found some bugs in the JS runner. Looks good? OK to squash? |
snim2
reviewed
Feb 7, 2018
| int | ||
| main(int argc, char **argv) | ||
| { | ||
| char *krun_benchmark = 0; | ||
| int krun_total_iters = 0, krun_param = 0, krun_iter_num = 0; | ||
| int krun_debug = 0, krun_num_cores = 0, krun_core; | ||
| int krun_debug = 0, krun_num_cores = 0, krun_core, krun_instrument = 0; |
There was a problem hiding this comment.
Is there a reason behind not initialising krun_core?
There was a problem hiding this comment.
We can do, although it will be initialised before its first use in the loop header:
for (krun_core = 0; krun_core < krun_num_cores; krun_core++) {
There was a problem hiding this comment.
OK, it just seems weird to be the odd one out on that line.
snim2
reviewed
Feb 7, 2018
| def usage(): | ||
| print(__doc__) | ||
| sys.exit(1) | ||
|
|
||
| # main |
|
OK, a couple of extremely minor things. It warms my heart to see some new tests. |
|
I don't think I made any new tests, but we could test |
|
That sounds like a good idea. |
|
Here we are. Found a documentation bug in doing so. Ready for squash? |
|
If there's nothing else we can sensibly test then yes, please squash! |
|
I think when we come to re-write the existing instrumentation we can write some more tests, but not at this moment. |
Passes extra arguments to the iterations runner which is then expected to write directly into the instrumentation directory. Before Krun would scrape stderr from the pipe it opens to the iterations runner, before dumping it into a file on behalf of the runner. That code still exists as we have instrumentation using it, however, this should be ported to the new API soon.
vext01
force-pushed
the
runners-more-info
branch
from
f38d6b2 to
7d4e626
Compare
|
Splat. |
snim2
approved these changes
Feb 7, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DO NOT MERGE.
This is the beginning of a fix for #366
We pass two extra flags to the iterations runners only if we are in instrumentation mode. Those flags are:
nbody:PyPy:default-python.The plumbing on this one hurt my head. We will need to run some more tests, probably using the warmup experiment.
Before I go and update all of the other iterations runners, is this what we want?