implement high contention allocator #122
Conversation
foundationdb/src/hca.rs
Outdated
| subspace: &Subspace, | ||
| ) -> Result<Subspace, Error> { | ||
| loop { | ||
| let range_option = RangeOptionBuilder::from(self.counters.range()) |
There was a problem hiding this comment.
one question I had: this RangeOption gets remade a bunch of times, what's a good way to dedupe that work? Should it be set as a const and update get_ranges to work over references?
There was a problem hiding this comment.
Generally speaking, it might be easier to create a RangeOptionRef type then trying to make the existing one generic over both owned and reference types. I’ll need to refresh my memory though and review the code. I’ll try and do that ASAP.
There was a problem hiding this comment.
Reviewing the code, this would be a good change, to make a RangeOptionRef that could be constructed directly from the two vecs from get_ranges(). I'm not sure why that is returning Vecs. This might end up being a rather large and deep change though.
There was a problem hiding this comment.
One additional tricky piece about this method I later learned: RangeOptionBuilder::from(subspace.range()) compiles, but since range() returns (Vec<u8>, Vec<u8>), and because we have tuple definitions for pairs + Vec<u8> Elements, it actually gets interpreted as a Tuple. This then produces a Range that spans [(begin, end, 0x00), (begin, end, 0xFF)).
Maybe we could add a key/range-convertible trait that both tuples / subspaces implement that returns (KeySelector, KeySelector)
There was a problem hiding this comment.
For either case, I might hold off on these changes to keep this PR scoped down
There was a problem hiding this comment.
Sorry for the late reply. Yes, I'm sure this could be cleaned up quite a bit.
There was a problem hiding this comment.
It would be great to add a test for this feature. See: https://github.com/bluejekyll/foundationdb-rs/tree/master/foundationdb/tests
Otherwise, there are a few questions and one thing that could be cleaned up. I also recommend running cargo clippy, it might offer some hints about ways to improve the code.
foundationdb/src/hca.rs
Outdated
| if start < 65535 { | ||
| return 1024; | ||
| } | ||
| return 8192; |
There was a problem hiding this comment.
This block could be a little cleaner in Rust as:
match start {
_ if start < 255 => 64,
_ if start < 65535 => 1024,
_ => 8192,
}
foundationdb/src/hca.rs
Outdated
| subspace: &Subspace, | ||
| ) -> Result<Subspace, Error> { | ||
| loop { | ||
| let range_option = RangeOptionBuilder::from(self.counters.range()) |
There was a problem hiding this comment.
Reviewing the code, this would be a good change, to make a RangeOptionRef that could be constructed directly from the two vecs from get_ranges(). I'm not sure why that is returning Vecs. This might end up being a rather large and deep change though.
foundationdb/src/hca.rs
Outdated
| let kvs = range_result.key_values(); | ||
|
|
||
| for kv in kvs.as_ref() { | ||
| if let Element::I64(counter) = self.counters.unpack(kv.key()).expect("unable to unpack counter key") { |
There was a problem hiding this comment.
Should this be an error rather than a panic?
foundationdb/src/hca.rs
Outdated
| const ONE_BYTES: &[u8] = &[0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; | ||
|
|
||
| /// High Contention Allocator | ||
| /// |
There was a problem hiding this comment.
It would be great to have a bit more documentation (perhaps from the FoundationDB docs themselves) that describes this feature.
There was a problem hiding this comment.
yes, will get to this next 👍
foundationdb/src/hca.rs
Outdated
| use tuple::Element; | ||
|
|
||
| lazy_static! { | ||
| static ref LOCK: Mutex<i32> = Mutex::new(0); |
There was a problem hiding this comment.
Is this lock truly global? or should it be limited to each instance of an HCA?
There was a problem hiding this comment.
still not sure about this one. I'll dig into the other bindings some more
|
Added some tests and got it working correctly. I'll take another pass at docs + looking into the mutex behavior in the next few days. Thank you for the comments! |
|
Added some docs, and moved the mutex out of global state and into the HCA. From my understanding, the mutex is to prevent an individual transaction from racing if it has concurrent calls into the HCA. The Java + Go bindings use a global mutex, the Python + Ruby bindings create a transaction-local variable to lock on, and the Flow bindings don't lock at all. All this to say, an HCA-mutex ought to work fine here. |
|
This all looks good up to this point. We maybe want to revisit the design of the function for the windows creation logic in the future. I think that could probably be more elegantly modeled as a set of state machines, but not necessary at this point. It looks like some of the tests are failing. I’m happy to merge once those are passing unless @yjh0502 has any concerns. |
|
Still getting used to the idea of builds failing from doc formatting! Fixed the comments. |
|
Just got a quick sec. This looks good. Thanks for the PR! Any interest in being a maintainer on the project? It's not at the top of my mind at the moment. It could be moved to a more neutral organization if that's desirable. @yjh0502 any concerns? |
|
@bluejekyll I could try 😄 I'm still learning rust, but I have a decent grasp of the FDB API |
Hey folks, I took a stab at writing the HCA (#44) for the
DirectoryLayerand was looking for some early feedback on it. I've never written Rust before, so I didn't want to go too deep into writing up docs if there's still a lot to iterate on code-wise 😅The implementation is pretty much just porting over the Go bindings. From a brief reading, it looks like more of the
DirectoryLayerneeds to be implemented before we can use thebindingtesterto verify correctness.@bluejekyll @yjh0502