Add Unstable Options to the CLI #1213

shemnon · 2019-04-03T23:40:29Z

PR description

Create a standard way for various modules to expose unstable options such as performance related options to the CLI but not have it exposed in the help menu.

Each module defines a PerformanceOptions class and the Pantheon command mixes it in.

ajsutton · 2019-04-04T00:24:27Z

I generally like this approach, particularly the ability to provide options we don't guarantee will be around forever. It would be nice if there was a way to get help for all of these performance related options (like java -X gives).

…ions

multiple modules help option

shemnon · 2019-04-04T19:19:15Z

Here's what the --X help output looks like.

Performance options for RocksDBPerformanceOptions
      --Xrocksdb-max-open-files=<INTEGER>
         Max number of files RocksDB will open (default: 1024)

Performance options for SynchronizerPerformanceOptions
      --Xsynchronizer-computationParallelism=<INTEGER>
         Performance config for computationParallelism (default: # of processors)
      --Xsynchronizer-downloaderChainSegmentSize=<INTEGER>
         Performance config for downloaderChainSegmentSize (default: 200)
      --Xsynchronizer-downloaderChainSegmentTimeoutsPermitted=<INTEGER>
         Performance config for downloaderChainSegmentTimeoutsPermitted (default: 5)
      --Xsynchronizer-downloaderChangeTargetThresholdByHeight=<LONG>
         Performance config for downloaderChangeTargetThresholdByHeight (default: 20)
      --Xsynchronizer-downloaderCheckpointTimeoutsPermitted=<INTEGER>
         Performance config for downloaderCheckpointTimeoutsPermitted (default: 5)
      --Xsynchronizer-downloaderHeaderRequestSize=<INTEGER>
         Performance config for downloaderHeaderRequestSize (default: 200)
      --Xsynchronizer-downloaderParallelism=<INTEGER>
         Performance config for downloaderParallelism (default: 4)
      --Xsynchronizer-fastSyncFullValidationRate=<FLOAT>
         Performance config for fastSyncFullValidationRate (default: 0.1)
      --Xsynchronizer-fastSyncPivotDistance=<INTEGER>
         Performance config for fastSyncPivotDistance (default: 50)
      --Xsynchronizer-transactionsParallelism=<INTEGER>
         Performance config for transactionsParallelism (default: 2)
      --Xsynchronizer-worldStateHashCountPerRequest=<INTEGER>
         Performance config for worldStateHashCountPerRequest (default: 348)
      --Xsynchronizer-worldStateMaxRequestsWithoutProgress=<INTEGER>
         Performance config for worldStateMaxRequestsWithoutProgress (default: 1000)
      --Xsynchronizer-worldStateMinMillisBeforeStalling=<LONG>
         Performance config for worldStateMinMillisBeforeStalling (default: 5)
      --Xsynchronizer-worldStateRequestParallelism=<INTEGER>
         Performance config for worldStateRequestParallelism (default: 10)

ajsutton

I really like the idea of this - will make it much easier to experiment and tune the various values and allow clients to tweak them when needed.

We'll need to make some of the options in SynchronizerConfig clearer but I think that's separate to this PR and probably makes more sense once the chain downloader is fully in place since it will change things a little.

ajsutton · 2019-04-04T21:09:49Z

...th/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerPerformanceOptions.java

+      Suppliers.memoize(SynchronizerPerformanceOptions::new);
+
+  public static SynchronizerPerformanceOptions get() {
+    return cachedInstance.get();


Is there any way we can avoid using a singleton here? It's probably ok in the command line stuff since it's one-pantheon-per-vm but it's so much simpler to just have a blanket avoid singletons rule.

Not easily in the general case. For the synchronizer I can add this to the SynchronizerConfiguration. For RocksDB... there is no good configuration to hang it off. And then how much about the details of how pantheon works should the command shell know?

The best answer I see you may not like: DI such as Guice or Dagger. An external module could tell the DI module that it provides a set of configuration mixins. These are singleton within the scope of the injector, so the RocksDB module can provide it and use it and the command shell just passes the values. But even without Guide and Dagger we could do this in a generic fashion but we quickly produces the services that Guice and Dagger are already providing.

In terms of how much the command shell should know about how pantheon works, I think its one job is to setup the configuration for Pantheon. So anything we expose on the command line should be also available as a config option when you use "embedded" Pantheon via RunnerBuilder.

ajsutton · 2019-04-04T21:14:50Z

...eum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java

-    private int worldStateHashCountPerRequest = DEFAULT_WORLD_STATE_HASH_COUNT_PER_REQUEST;
-    private int worldStateRequestParallelism = DEFAULT_WORLD_STATE_REQUEST_PARALLELISM;
+    private long downloaderChangeTargetThresholdByHeight =
+        SynchronizerPerformanceOptions.get().downloaderChangeTargetThresholdByHeight;


Using the singleton here probably is bad since SynchronizerConfig.Builder is used when creating multiple instances in the same VM. We probably want it to be completely isolated from the command line parsing. Probably going to create a bunch more boiler plate unfortunately. I think having a clear separation between command line and runtime code is probably worth it.

This may be the correct time to look at DI again.

Maybe, but if you can do it with DI you can do it without - it's just a question of how many places you have to pass the value through. Showing the two different approaches in this particular situation is probably a good way to evaluate DI...

+1 for the DI evaluation

Won't build.

* Move options into configuration objects * Create a RocksDbConfiguration object * Chase down all the points the new RocksDbConfiguration object matters.

ajsutton

Few suggested tweaks but LGTM.

ajsutton · 2019-04-05T06:18:12Z

...eum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java

+    @CommandLine.Option(
+        names = "--Xsynchronizer-downloader-change-target-threshold-by-height",
+        hidden = true,
+        defaultValue = "20",


I take it we wound up having to have the defaults duplicated then? Should we raise an issue for PicoCLI so hopefully we can have them magically picked up at some point in the future?

I didn't look too deeply. There is support for a defaults provider but getting it linked to the fields that were annotated is a good bit of work.

ajsutton · 2019-04-05T06:18:42Z

...eum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java

+    //      paramLabel = "<UINT256>",
+    //      description =
+    //          "Minimum total difficulty difference before switching fast sync download peers
+    // (default: ${DEFAULT-VALUE})")


nit: Probably should just remove the annotation instead of commenting it out.

I added support for UInt256 and restored this annotation.

ajsutton · 2019-04-05T06:21:59Z

...eum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java

+        defaultValue = "10",
+        paramLabel = "<INTEGER>",
+        description =
+            "Number of threads committed to fast sync world state downloading (default: ${DEFAULT-VALUE})")


Suggested change

"Number of threads committed to fast sync world state downloading (default: ${DEFAULT-VALUE})")

"Number of concurrent requests to use when downloading fast sync world state (default: ${DEFAULT-VALUE})")

There's only ever one thread used, just multiple concurrent requests.

ajsutton · 2019-04-05T06:25:37Z

...eum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java

+        defaultValue = "1000",
+        paramLabel = "<INTEGER>",
+        description =
+            "Number of world state requests accepted without progress before stopping (default: ${DEFAULT-VALUE})")


Suggested change

"Number of world state requests accepted without progress before stopping (default: ${DEFAULT-VALUE})")

"Number of world state requests accepted without progress before considering the download stalled (default: ${DEFAULT-VALUE})")

ajsutton · 2019-04-05T06:26:12Z

...eum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/SynchronizerConfiguration.java

+        defaultValue = "5",
+        paramLabel = "<LONG>",
+        description =
+            "Minimum time in ms to permit world state queries to stall (default: ${DEFAULT-VALUE})")


Suggested change

"Minimum time in ms to permit world state queries to stall (default: ${DEFAULT-VALUE})")

"Minimum time in ms without progress before considering a world state download as stalled (default: ${DEFAULT-VALUE})")

ajsutton · 2019-04-05T06:30:18Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/UnstableOptionsSubCommand.java

+        .forEach(option -> cs.addOption(option.toBuilder().hidden(false).build()));
+
+    // Print out the help text.
+    out.printf("Unstable options for %s%n", mixinName);


The help output includes Unstable options for standaloneCommands at the top so we probably want to exclude that module as well (or probably even better, exclude all modules that have no unstable options).

Fixed this with a change of approach: don't list mixins with zero relevant options. That way I don't have to enumerate help or standalone commands.

…ions

rough draft.

caf08ed

Each module defines a PerformanceOptions class and the Pantheon command mixes it in.

shemnon added 2 commits April 4, 2019 09:13

Merge branch 'master' of github.com:PegaSysEng/pantheon into perf_opt…

45ec604

…ions

second draft

099ae10

multiple modules help option

ajsutton reviewed Apr 4, 2019

View reviewed changes

shemnon added 3 commits April 4, 2019 19:03

options in builder.

ee1fa56

Won't build.

third draft

54ebe51

* Move options into configuration objects * Create a RocksDbConfiguration object * Chase down all the points the new RocksDbConfiguration object matters.

restore a changed defualt

a35e35a

shemnon changed the title ~~Add Performance Options to the CLI~~ Add Unstable Options to the CLI Apr 5, 2019

shemnon marked this pull request as ready for review April 5, 2019 05:37

ajsutton approved these changes Apr 5, 2019

View reviewed changes

shemnon added 4 commits April 5, 2019 07:36

Merge branch 'master' of github.com:PegaSysEng/pantheon into perf_opt…

28daa88

…ions

requested review cahnges.

d9289f4

fix broken benchmark classes

9c055f2

correct default˚

887eb08

shemnon merged commit bfb3d56 into PegaSysEng:master Apr 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Unstable Options to the CLI #1213

Add Unstable Options to the CLI #1213

shemnon commented Apr 3, 2019 •

edited

Loading

ajsutton commented Apr 4, 2019

shemnon commented Apr 4, 2019

ajsutton left a comment

ajsutton Apr 4, 2019

shemnon Apr 4, 2019

ajsutton Apr 4, 2019

ajsutton Apr 4, 2019

shemnon Apr 4, 2019

ajsutton Apr 4, 2019

AbdelStark Apr 5, 2019

ajsutton left a comment

ajsutton Apr 5, 2019

shemnon Apr 5, 2019

ajsutton Apr 5, 2019

shemnon Apr 5, 2019

ajsutton Apr 5, 2019

ajsutton Apr 5, 2019

ajsutton Apr 5, 2019

ajsutton Apr 5, 2019

shemnon Apr 5, 2019

	"Number of threads committed to fast sync world state downloading (default: ${DEFAULT-VALUE})")
	"Number of concurrent requests to use when downloading fast sync world state (default: ${DEFAULT-VALUE})")

	"Number of world state requests accepted without progress before stopping (default: ${DEFAULT-VALUE})")
	"Number of world state requests accepted without progress before considering the download stalled (default: ${DEFAULT-VALUE})")

	"Minimum time in ms to permit world state queries to stall (default: ${DEFAULT-VALUE})")
	"Minimum time in ms without progress before considering a world state download as stalled (default: ${DEFAULT-VALUE})")

Add Unstable Options to the CLI #1213

Add Unstable Options to the CLI #1213

Conversation

shemnon commented Apr 3, 2019 • edited Loading

PR description

ajsutton commented Apr 4, 2019

shemnon commented Apr 4, 2019

ajsutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajsutton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon commented Apr 3, 2019 •

edited

Loading