Page MenuHomeSoftware Heritage

Add proper CLI args management in benchmarking tools
ClosedPublic

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

Details

Summary

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

Diff Detail

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.

Event Timeline

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.

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.

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?

This revision is now accepted and ready to land.Aug 14 2019, 9:37 PM
This revision was landed with ongoing or failed builds.Aug 14 2019, 9:39 PM
This revision was automatically updated to reflect the committed changes.