feat: Added DB configuration option to turn on/off parallel insert and hashing #1340
Conversation
…s/firewood into bernard/insert-worker-pool-rayon
worker thread compiles (not tested).
a new branch node with an empty partial path is added to support parallel insertion. Appears to be working.
trie after the insert operations.
…nard/parallel-hashing
…nard/parallel-hashing
…nard/parallel-hashing
| return newDatabaseConfig(dbFile, conf) | ||
| } | ||
|
|
||
| func newDatabaseConfig(dbFile string, conf *Config) (*Database, func() error, error) { |
There was a problem hiding this comment.
nit:
| func newDatabaseConfig(dbFile string, conf *Config) (*Database, func() error, error) { | |
| func newDatabaseWithConfig(dbFile string, conf *Config) (*Database, func() error, error) { |
| FreeListCacheEntries: 40_000, | ||
| Revisions: 100, | ||
| ReadCacheStrategy: OnlyCacheWrites, | ||
| Parallel: false, |
There was a problem hiding this comment.
If there is no functionality change and is a performance improvement, then why is this available to be set? Just to ensure that it works as expected prior to actually shipping it? It seems like this shouldn't even be available
There was a problem hiding this comment.
There are some cases (e.g., very small proposals) in which parallel insertion/hashing is slower than serial insertion/hashing. We may want to allow the application using firewood to choose which version they want to use.
We are still waiting on some performance numbers to determine if providing this option is necessary.
There was a problem hiding this comment.
I think we should hold off on merging this until we know what the tradeoffs are. It's an easy change we can make, but nobody is going to want to manually configure this variable I think.
|
|
||
| func newTestDatabase(t *testing.T) *Database { | ||
| conf := DefaultConfig() | ||
| conf.Truncate = true // in tests, we use filepath.Join, which creates an empty file |
There was a problem hiding this comment.
Why is this necessary? Seems like it must have been working before...
There was a problem hiding this comment.
newTestDatabase previously called newDatabase, which sets the Truncate flag to be true.
In this PR, newTestDatabase calls newTestDatabaseConfig which calls newDatabaseConfig instead of newDatabase. None of the *Config functions sets the truncate flag since they take the configuration as a parameter. Because of this, I now set the truncate flag in newTestDatabase.
I could instead set truncate to be true in newDatabaseConfig, but I thought that might be more confusing.
| return newTestDatabaseConfig(conf, t) | ||
| } | ||
|
|
||
| func newTestDatabaseConfig(conf *Config, t *testing.T) *Database { |
There was a problem hiding this comment.
nit:
| func newTestDatabaseConfig(conf *Config, t *testing.T) *Database { | |
| func newTestDatabaseConfig(t *testing.T, conf *Config) *Database { |
| func TestProposalUsingParallel(t *testing.T) { | ||
| r := require.New(t) | ||
| conf := DefaultConfig() | ||
| conf.Truncate = true |
There was a problem hiding this comment.
I don't think this is necessary
There was a problem hiding this comment.
Since I'm calling newTestDatabaseConfig instead of newTestDatabase which uses the passed in configuration directly, I believe I need to manually set the truncate flag here (since the default is false). Perhaps I'm misunderstanding your suggestion.
| FreeListCacheEntries: 40_000, | ||
| Revisions: 100, | ||
| ReadCacheStrategy: OnlyCacheWrites, | ||
| Parallel: false, |
There was a problem hiding this comment.
I think we should hold off on merging this until we know what the tradeoffs are. It's an easy change we can make, but nobody is going to want to manually configure this variable I think.
Depends on #1303. Addresses issue #1335. List of changes:
Adds an option to DB config and configuration manager to turn on/off parallel propose when opening the database. If the option is off, then the threadpool option is set to None and normal propose is used. The current default is off, but the test_proposal_parallel test case always turns the option on.
Adds a "parallel_merkle" fastrace::span for parallel propose.
Adds FFI changes to allow golang to turn on or off parallel inserts/hashing when opening a DB. Includes a basic test.