[NC-2100] Parallel Processing File Import Performance #683
Conversation
shemnon
changed the title
| private final Semaphore blockBacklog = new Semaphore(2); | ||
|
|
||
| private final ExecutorService validationExecutor = Executors.newCachedThreadPool(); | ||
| private final ExecutorService importExecutor = Executors.newSingleThreadExecutor(); |
There was a problem hiding this comment.
These executors don't seem to be linked into the Pantheon life cycle so they get shutdown appropriately? That will lead to thread leaks and potentially crashes if something in those threads tries to access RocksDB after it has been closed.
There was a problem hiding this comment.
This is a command line util, called directly from BlocksSubCommand, so it won't be called except from the CLI and it will be in total ownership of the JVM. (Different story for the networked version of this patch). There is no pantheon life cycle for this class.
As for the RocksDB error, I already saw it when I didn't wait for the last block to finish. By joining on the previousBlockFuture after the loop where all the contents of the file are read in I ensure that all submitted blocks are complete and hence all the executors are emptied since the last block depends on all previous blocks and all of those depend on what was in the executors. I can add shutdown and awaitTermination to the executors but it would just be ceremony as they are already emptied at that point.
There was a problem hiding this comment.
Ah yes, sorry I had missed that this was the util BlockImporter not the protocol spec one.
| try { | ||
| blockBacklog.acquire(); | ||
| } catch (final InterruptedException e) { | ||
| LOG.error(e); |
There was a problem hiding this comment.
Sadly this will lose the stack trace from the error - the first arg in log4j is Object so it just calls toString.
| LOG.error(e); | |
| LOG.error("", e); |
Or provide a meaningful message if there is one to provide, but empty string is enough to get what you were expecting.
| try { | ||
| validationExecutor.awaitTermination(5, SECONDS); | ||
| } catch (final Exception e) { | ||
| LOG.error(e); |
There was a problem hiding this comment.
Same log4j trap as above.
| try { | ||
| importExecutor.awaitTermination(5, SECONDS); | ||
| } catch (final Exception e) { | ||
| LOG.error(e); |
There was a problem hiding this comment.
Same log4j trap as above.
| otherFuture = extractingFuture; | ||
| } else { | ||
| otherFuture = | ||
| extractingFuture.runAfterBothAsync(previousBlockFuture, () -> {}, importExecutor); |
There was a problem hiding this comment.
Couldn't this just be:
| extractingFuture.runAfterBothAsync(previousBlockFuture, () -> {}, importExecutor); | |
| CompletableFuture.allOf(extractingFuture, previousBlockFuture); |
There was a problem hiding this comment.
I'm not sure if it will interact with the particular executor it is waiting on or not. I'll benchmark it before submitting.
| private final Semaphore blockBacklog = new Semaphore(2); | ||
|
|
||
| private final ExecutorService validationExecutor = Executors.newCachedThreadPool(); | ||
| private final ExecutorService importExecutor = Executors.newSingleThreadExecutor(); |
There was a problem hiding this comment.
Ah yes, sorry I had missed that this was the util BlockImporter not the protocol spec one.
try CompletableFuture.allOf
shemnon
changed the title
PR description
Improve performance of file based import by spinning off ECDSA and block validation into separate threads. The result are then joined and EVM execution and block import are performed synchronously.
Fixed Issue(s)
NC-2100