Add proper CLI args management in benchmarking tools
ClosedPublic
Actions

Authored by haltode on Aug 14 2019, 2:53 PM.

Details

Reviewers

zack
seirl

Commits

Summary

Benchmark tools now accept options such as 'nb-nodes' and 'seed' instead
of default hard-coded values.

Repository

rDGRPH Compressed graph representation

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Build has FAILED

zack requested changes to this revision.Aug 14 2019, 4:59 PM

zack added inline comments.

java/server/src/main/java/org/softwareheritage/graph/benchmark/Common.java
53–54	I'm assuming 0 here is the default seed value that will be used if it's not given on the command line. The default for random seed should not be zero (or anything fixed), rather it should be something backed by entropy. As I understand from the java documentation, the way to obtain that is simply not passing an argument to the `Random` constructor. So you should change this to make sure that, if "seed" is not passed on the CLI, no constructor is passed to `Random`.

This revision now requires changes to proceed.Aug 14 2019, 4:59 PM

Use default Random constructor if no seed is passed by CLI argument.

Build has FAILED

zack requested changes to this revision.Aug 14 2019, 9:25 PM

zack added inline comments.

java/server/src/main/java/org/softwareheritage/graph/benchmark/AccessEdge.java
32	Cheating ! ;-) "-1" is a perfectly valid seed that can be passed on the CLI, so you shouldn't rely on it being a special value given you do not control the input. You should make the default value for the seed `null` (it seems to be valid in JSAP) and check whether that value is `null` as condition here.

This revision now requires changes to proceed.Aug 14 2019, 9:25 PM

java/server/src/main/java/org/softwareheritage/graph/benchmark/AccessEdge.java
32	The problem with `null` is that in Java only wrapper class for data types can be checked against `null`, so in this case `Long` instead of `long` which I found a bit weird compared to the -1 "trick" :p

Use null instead of -1 value.

Build has FAILED

java/server/src/main/java/org/softwareheritage/graph/benchmark/AccessEdge.java
32	You can either use a `Long benchArgs.seed` attribute and use `getLong()` later, or, have a `benchArgs.rng` instead of `.seed` and initialize it earlier on instead of using `config.getLong`. Or am I missing something?