Auto close feature to improve memory management of option types. #57
Conversation
…ions when not cleaned up manually.
rocksdb/transactiondb.nim
Outdated
| readOpts = ReadOptionsRef(nil), | ||
| writeOpts = WriteOptionsRef(nil), | ||
| txDbOpts = TransactionDbOptionsRef(nil), | ||
| readOpts = defaultReadOptions(), |
There was a problem hiding this comment.
if this is called without supplying readOpts/writeOpts, what happens? Who frees the options allocated by the default?
There was a problem hiding this comment.
ie in the colfamily pr, the idea was that if nil is given, the temporary options instance is "owned" by the callee (beginTransaction in this case) and therefor it also deallocates it
There was a problem hiding this comment.
They get freed when the db instance is closed. db.close() will free all the opts types regardless of whether passed in or not.
There was a problem hiding this comment.
Actually the above comment applies in the other locations. In this instance the opts are freed when the transaction is closed.
There was a problem hiding this comment.
what if the transaction/database/etc fails to open?
There was a problem hiding this comment.
I think the core issue here is that when we do xxx = defaultOptions in the parameter list, the ownership is unclear which has these knock-on effects. semantically, it becomes strange that "sometimes" the function closes the passed-in options and sometimes it doesn't (if I pass in options, can I use them for another call? etc)
I'm not sure this is worth fixing, but it's certainly worth thinking about and the nil approach was a compromise (non-nil = caller owns) to get there.
There was a problem hiding this comment.
I actually found the solution using nil to be more unclear which is why I removed it. Using the default options solution makes it clear that the options will always get cleaned up when the database is closed. When using the nil method, the options require manual clean up if being passed in, if they don't get passed in then the options are cleaned up in the open function before the database is closed (this may cause other issues). The Java RocksDb library states ' Options instance should not be disposed before all DBs using this options instance have been closed', see here: https://javadoc.io/doc/org.rocksdb/rocksdbjni/latest/org/rocksdb/RocksDB.html#open-org.rocksdb.DBOptions-java.lang.String-java.util.List-java.util.List-
You should be able to use options for multiple calls as long as the database is open so for most of the functions that won't be a problem. In the case of beginTransaction these options get cleaned up when closing the transaction and so I guess this could be a problem if we want to reuse the opts across transactions. This function is not currently used in Nimbus by the way.
There was a problem hiding this comment.
I'm not sure if we can use nim destructors with the refc garbage collector but perhaps in a future change we can use destructors to help cleanup the opts types and then don't close any opt types in db.close().
There was a problem hiding this comment.
I'm not sure if we can use nim destructors
no, these are ORC-only. we can indeed revisit the problem then - until then, perhaps it's worth documenting these quirks in the API as well, similar to java.
There was a problem hiding this comment.
Sure, I'll add that.
I'm also working a change to support choosing if the types are freed when closing the database or not by adding a flag to each opt type. If you pass in an opt it won't get closed automatically but if none is passed in the default opts will get closed when the database is closed.
bhartnett
changed the title
This change reverts some of the changes from #48 which prevents the opts types from be closed when passed in. Currently in Nimbus the db opt types are not freed manually so there is a memory leak from what I can tell. This change should fix the problem once we roll this version into Nimbus.
Also doing a bit of cleanup and add a few more tests.