using arc'd tmpfile to fix bal(1M) benchmark #637
Conversation
Voxelot
changed the title
|
If we take that approach, we might create a shared resource that will impact on each iteration of the benchmark. What if we keep the problem split, first merge #630 and then make another PR to handle this issue? This way, we don't aggregate different responsibilities to the original PR |
That is already happening right now in the current implementation of #630, the only difference this PR introduces is making the tempfile safely clonable like everything else in VmBenchPrepared. Currently, the tempfile lifetime is being managed by the VmBenchPrepared instance, since that is where it's created. Since these VmBenchPrepared instances are cloned and dropped frequently, and they have a shorter lifetime than the shared rocksdb instance being passed around, this is causing the tempfile to drop prematurely. In order to ensure each test has it's own isolated db, when should be preparing the VM during each setup instead of cloning instances of the prepared state. |
| fn new_db() -> io::Result<Database> { | ||
| TempDir::new().and_then(|t| Database::open(t.as_ref()).map_err(|e| e.into())) | ||
| fn new_db() -> Database { | ||
| // when rocksdb is enabled, this creates a new db instance with a temporary path |
There was a problem hiding this comment.
Should fuel-core's rocksdb feature be enabled by default on this benchmark then? That way no benchmarks are accidentally done on much more performant RAM databases?
| TempDir::new().and_then(|t| Database::open(t.as_ref()).map_err(|e| e.into())) | ||
| fn new_db() -> Database { | ||
| // when rocksdb is enabled, this creates a new db instance with a temporary path | ||
| Database::default() |
There was a problem hiding this comment.
Maybe we just delete this new_db function and instead unwrap_or_default?
use
Database::defaultto get a tmpfile rocksdb db instance. This solves problems related to prematurely dropping the tempfile by using an Arc.