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

Use rule target kronecker.txt and remove from macro_bench #209

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shakthimaan
Copy link
Contributor

The graph500seq benchmarks were failing because of kronecker.txt: No such file or directory as mentioned at #207. This PR

  • Uses a target rule to build kronecker.txt before running kernel2 and kernel3.
  • Removes graph500seq benchmarks from macro_bench tag.

@kayceesrk
Copy link
Contributor

What's the thought process behind removing the macro_benchmark tag? Do these benchmarks not run long enough?

@shakthimaan
Copy link
Contributor Author

shakthimaan commented Mar 25, 2021

What's the thought process behind removing the macro_benchmark tag? Do these benchmarks not run long enough?

Yes, they do not run long enough, and hence the benchmark graphs are getting skewed. Hence, the request to remove them from macro_bench.

@kayceesrk
Copy link
Contributor

If that's the case then the timing tags 1s_10s etc should also be removed?

@Sudha247
Copy link
Contributor

This is one of the results I got a while ago of graph500 benchmarks:

{"name":"kernel1.12_10","command":"taskset --cpu-list 5 ./kernel1.exe 12 10","time_secs":0.0026328563690185547,"user_time_secs":0.002383,"sys_time_secs":0.0,"maxrss_kB":5600,"ocaml":{"version":"4.10.0+multicore","c_compiler":"gcc","architecture":"amd64","word_size":"64","system":"linux","stats":"false","function_sections":"true","supports_shared_libraries":"true"},"gc":{"allocated_words":1874,"minor_words":1808,"promoted_words":0,"major_words":66,"minor_collections":0,"major_collections":0,"heap_words":4096,"top_heap_words":4096,"mean_space_overhead":0.0},"codesize":206811.0,"ocaml_url":"https://github.com/Sudha247/ocaml-multicore/archive/3deeb2604320a7337cc75f20116ea237d65fc400.tar.gz"}

Might be the case that the benchmark was not running due to an absent input file and this PR fixes it. But I did not see an error message saying the benchmark failed to run, it's possible the error message was not printed. We may have to revisit the timing tags for this. Also, wrt macro_bench tag, it will be good to confirm the results are not fluctuating (which I observed a few times) before adding it again as a macro benchmark.

@kayceesrk
Copy link
Contributor

Can we confirm the running times before removing the macro_bench tag? If it does run for the amount of time it says it does, then I'd prefer to keep the macro_bench tag.

If there is variance between different runs, it may be due to the random number generation. We should fix the seed to a constant value if that is the case.

@Sudha247
Copy link
Contributor

I'm happy with retaining the tag if the benchmarks are running long enough. So, I think what needs to be done is:

  • Running the benchmarks with the target rule in this PR and getting the times for graph500 benchmarks, I believe this would give us accurate timing data. Once we have the results, timing tags might need to be adjusted accordingly.
  • Running the benchmarks some number of times to be sure there isn't much variation between different runs. All good if there is not much variation, in case there is this could become a source of noise in overall data and we need to find a way to fix it.

@kayceesrk
Copy link
Contributor

My assessment is that we should leave the macro_bench tag in. kernel1 runs for about 4s (with changes; see below), kernel2 runs for 25 seconds, and kernel3 runs for 80 seconds. They also do a fair number of major and minor collections.

Looks like kernel1 does not actually run the main function, but only defines it. See https://github.com/ocaml-bench/sandmark/blob/master/benchmarks/graph500seq/kernel1.ml#L106-L113. It is unsurprising it finishes immediately.
An easy fix would be to rename kernel1.ml to graph_construction.ml and implement a new kernel1.ml that calls linkKronecker() function. Reading the command line arguments for scale and edgefactor should be moved to the new kernel1.ml from the graph_construction.ml. This would of course mean that linkKronecker function would take scale and edge factors as arguments.

It seems weird that all the kernels take command-line arguments, currently set as 12 10 in run_config.json, and the kernels in addition take the kronecker.txt. Currently, kronecker.txt is generated from kronecker.exe 12 10. The interface seems brittle. Surely, we shouldn't have to pass 12 10 as arguments to each of the executables. What would happen if I generate kronecker.txt with arguments 15 15 and then run the kernels with 12 10. Something must go wrong? If not, why? The whole thing smells a little fishy.

Kernel2 and kernel3 write to the standard output. We should redirect the output to /dev/null in run_config.json.

We should use snake case and not camel case to be consistent.

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.

3 participants