Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated examples directory #71

Merged
merged 3 commits into from
Oct 26, 2015
Merged

Updated examples directory #71

merged 3 commits into from
Oct 26, 2015

Conversation

snim2
Copy link
Collaborator

@snim2 snim2 commented Oct 26, 2015

This is an updated version of the old examples/ directory, which is a cut down version of the warmup benchmarks. The aim is to use this to test the work to resolve Issue #41 and reboot mode.

This isn't quite ready to merge, firstly it needs an examples/README.md. Secondly, running this on bencher5 results in:

$ cd krun/examples/benchmarks
$ make
for i in richards nbody ; do \
    echo "Building java benchmark ${i}..."; \
    cd /home/snim2/krun/examples/benchmarks/${i}/java && \
    CLASSPATH=../../../krun/iterations_runners/ javac *.java; \
    done
Building java benchmark richards...
KrunEntry.java:2: error: cannot find symbol
class KrunEntry implements BaseKrunEntry {
                           ^
  symbol: class BaseKrunEntry
KrunEntry.java:4: error: cannot find symbol
      (new richards()).runIter(param);
           ^
  symbol:   class richards
  location: class KrunEntry
2 errors
Building java benchmark nbody...
KrunEntry.java:2: error: cannot find symbol
class KrunEntry implements BaseKrunEntry {
                           ^
  symbol: class BaseKrunEntry
1 error
Makefile:8: recipe for target 'all' failed
make: *** [all] Error 1

which looks like the CLASSPATH has been incorrectly set?

@snim2 snim2 added this to the Ready to publish milestone Oct 26, 2015
@vext01
Copy link
Member

vext01 commented Oct 26, 2015

Parts of krun need to be compiled. There is a Makefile in the krun root directory. Since you need Java support, you will need to set some make variables on the command line. Look at the build-krun target in the warmup experiment Makefile for an example.

@vext01
Copy link
Member

vext01 commented Oct 26, 2015

You will may need to set a CLASSPATH too.

@snim2
Copy link
Collaborator Author

snim2 commented Oct 26, 2015

OK, this is now fixed, documented, and I have tested it on my laptop.

@vext01
Copy link
Member

vext01 commented Oct 26, 2015

This looks pretty good Sarah. Especially the much needed documentation. I'm going to add some comments. They will be pretty nit-picky -- feel free to accept or reject them as you see fit.

executed on two VMs (*cPython* and a standard *JVM* such as HotSpot).
Each benchmark is run for 5 iterations on the same VM, then the VM is
restarted and the benchmark is re-run for another 5 iterations.
We say that the benchmarks are *executed* twice, with 5 *iterations*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use a different number of iterations and executions, so as to help distinguish them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? 5 iterations, 2 executions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Fine.

@vext01
Copy link
Member

vext01 commented Oct 26, 2015

OK, done.

@ltratt
Copy link
Member

ltratt commented Oct 26, 2015

There are definite tabs & spaces issues in some of this stuff. We should use 4 spaces unless there's a compelling reason not to.

@vext01
Copy link
Member

vext01 commented Oct 26, 2015

Those are likely present in the original benchmarks. I leave it to you guys to decide whether to fix them. Diff distance vs. readability.

@ltratt
Copy link
Member

ltratt commented Oct 26, 2015

Readability is generally more important (there are lines next to each other with mixed tabs/spaces, which means it looks pretty bad).

@snim2
Copy link
Collaborator Author

snim2 commented Oct 26, 2015

These latest changes need a good squashing, but hopefully they address the issues in the comments.

The README.md is quite extensive now. Would it be better to put this information in krun/README and close Issue #57 ?

@ltratt
Copy link
Member

ltratt commented Oct 26, 2015

I really think the advance method needs reformatting. Checking again, I'm not even sure if tabs are the problem -- but the formatting is all off, whatever the reason is.


* Edit /etc/default/grub (e.g. `sudo gedit /etc/default/grub`)
* Add `isolcpus=X` to `GRUB_CMDLINE_LINUX_DEFAULT` (where `X` is an integer > 0)
* Run `sudo update-grub`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we also require intel pstate to be disabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all linux specific details of course.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I've stated "Debian-based systems"; I don't know how anything other than grub works!

@vext01
Copy link
Member

vext01 commented Oct 26, 2015

Looks good Sarah! Anything to add @ltratt?

@snim2
Copy link
Collaborator Author

snim2 commented Oct 26, 2015

@vext01 you are happy to have this documentation in the example/ directory?

This configuration can be found in the file `examples/example.krun`.

## Step 0: prepare the benchmarking machine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 0? Step 1?!

Anyway, I think we should say something like "on Linux, you can optionally set up some kernel arguments to obtain more reliable results". On non-Linuxes, either there are no equivalent options, or there are different ones. We don't want to make it seem it's Linux only though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but the options aren't optional on Linux, krun bails if the environment isn't set up like this :)

@vext01
Copy link
Member

vext01 commented Oct 26, 2015

@snim2 You could even move it up to the main README.md if you liked. I'm not fussy though. Up to you.

@snim2
Copy link
Collaborator Author

snim2 commented Oct 26, 2015

@vext01 OK, I think that would be sensible to close #57 but I'll raise a new PR for it

@snim2
Copy link
Collaborator Author

snim2 commented Oct 26, 2015

Ready to squash?

@ltratt
Copy link
Member

ltratt commented Oct 26, 2015

Perhaps my only comment is that we might want to make it a little clearer that some stuff is Linux specific and some isn't. [We'll soon have this running on OpenBSD.] I don't think we have to tell the non-Linux people what to do, as such, but we should make it clear that they're not excluded.

@snim2
Copy link
Collaborator Author

snim2 commented Oct 26, 2015

@ltratt OK. When we have BSD support I'll check that these examples work on the BSD machine, and raise a new PR to make any necessary changes and move the documentation across to the main README.

Sarah Mount added 2 commits October 26, 2015 14:36
… up. Notes are Linux-specific. Includes current list of supported VMs and platforms.
@ltratt
Copy link
Member

ltratt commented Oct 26, 2015

I'm happy with that approach.

@snim2
Copy link
Collaborator Author

snim2 commented Oct 26, 2015

OK, squashed

vext01 added a commit that referenced this pull request Oct 26, 2015
@vext01 vext01 merged commit 3d908a9 into master Oct 26, 2015
@vext01 vext01 deleted the updated-examples branch October 26, 2015 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants