Adds FileStore, an implementation of the Store and StoreView traits that uses a
file on disk to store the data, and reads and writes blocks using file I/O.
Details
Details
- Reviewers
jsgf durham - Group Reviewers
Restricted Project - Commits
- rFBHGXbbb86cb7c3c5: treedirstate: add FileStore
Diff Detail
Diff Detail
- Repository
- rFBHGX Facebook Mercurial Extensions
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
| rust/treedirstate/src/filestore.rs | ||
|---|---|---|
| 26 | Ditto comment from D1397 that usize isn't the right type for non-memory things. | |
| 28 | This is not safe for multiple current writers, is it? | |
| 30 | It's a pity there doesn't seem to be a standard equiv of pread/pwrite. | |
| 31 | Does BufWriter gain you much here? I could imagine that if you have a workload of lots of back-to-back appends of small objects it could help, but otherwise it seems to just complicate things with the seeking. | |
| 33–34 | I'm not very keen on keeping separate redundant state - this can always be computed fetching the file size, right? At the very least you should access it via a method so that you can change the underlying implementation, and/or debug_assert that its always consistent with the file size. | |
| 36–40 | Would it make sense to put this in the same refcell as the file? | |
| 66–75 | It might be nice to have a way to explicitly open it RO, rather than just make it a fallback. (open_ro/open_rw/open) | |
| 70 | Is .append(true) useful here? Probably not because you still need to track the filelen to get the keys and explicitly seek around for reads. | |
| 118 | Check for truncated write? | |
| 147 | Because you're doing things with set_len, I think it would be good to be explicit that it's Vec<u8>: let mut buffer: Vec<u8> = ... | |
| 152 | I tend to go with &mut buffer[..] | |
| rust/treedirstate/src/filestore.rs | ||
|---|---|---|
| 116 | Assert that data.len() fits into u32? | |
| rust/treedirstate/src/filestore.rs | ||
|---|---|---|
| 31 | It makes writes a *lot* faster, particularly initial writes of the full dirstate which is a lots of back-to-back appends. I benchmarked it both with and without, and it's definitely worth the extra compexity. | |
| 33–34 | Fetching the file size is a syscall, which is actually quite slow, and involves either seeking to the end, which messes up the caching, or stating the file, which is a bunch of extra work. The posix file I/O interface is actually pretty terrible. | |
| 36–40 | I'm not sure. I assume that would make the definition: file_data: RefCell<(BufWriter<File>, bool)>, and then I'd need to unpack it each time as: let (ref mut file, ref mut at_end) = self.file_data.get_mut(); I'm not sure that's any clearer, and it loses the name-based documentation of what at_end is. | |
| 66–75 | Nothing needs that right now, so I don't think we should do it. It should be fairly simple to add if/when the need arises. | |
| 70 | Yes, I decided to go for normal random-access I/O and control appending myself for this reason. | |
| 118 | Actually, I should be using write_all to make sure it all gets written. | |
| rust/treedirstate/src/filestore.rs | ||
|---|---|---|
| 28 | No, but at the moment it's only accessed by Python code with the GIL, so it's single-threaded anyway. | |
| rust/treedirstate/src/filestore.rs | ||
|---|---|---|
| 148–151 | Why not use Vec.resize() method and get rid of unsafe block? | |
| rust/treedirstate/src/filestore.rs | ||
|---|---|---|
| 25 | Someone more familiar with Rust might have a better idea, but I think this won't be possible until const fn is stabilized (and as long as slice::len is marked as const). | |
| 72 | Good spot. It should be &path in both places. It's required here because I use path twice. In create I only use it once so I get away with passing ownership. | |
| 148–151 | Is the compiler smart enough to optimize the implicit (and unnecessary) memset away? | |
| rust/treedirstate/src/errors.rs | ||
|---|---|---|
| 10–11 | Just curious: what's the difference between description() and display() methods? | |
| rust/treedirstate/src/filestore.rs | ||
| 33–34 | Have you decided not to add debug_asserts? | |
| 67 | What surprises me is that read_only is set inside the open() function. I thought that user should set this as a parameter to open() function. And if this parameter read_only is false, then open method should fail if it can't open it for writing. Or even have two separate methods - open_read_only() and open_read_write(). What do you think? | |
| 148–151 | Not sure | |
| rust/treedirstate/src/filestore.rs | ||
|---|---|---|
| 67 | The behaviour for open is to open read-write if possible, and read-only if the file is read-only. This gives the right behaviour for treedirstate, where it's possible to run commands that only read the dirstate in repos that you don't have write permission for. (There is a test for this, which is how I found it was necessary). I'm not opposed to open_rw and open_ro methods, but I don't need them for the treedirstate implementation, so I haven't added them. I'll make some notes for this in the doc comment for the function. | |
| rust/treedirstate/src/filestore.rs | ||
|---|---|---|
| 33–34 | I will add one in append like this: debug_assert!(self.position == file.seek(SeekFrom::End(0))?); | |
| rust/treedirstate/src/errors.rs | ||
|---|---|---|
| 10–11 | description() can only return &'static str, but display() returns String so can have variable bits in it. It's pretty confusing and not at all clear that both are useful. It's one of the things that the new error handling package failure does away with (it only has Display). | |
| rust/treedirstate/src/filestore.rs | ||
| 25 | You could go the other way and do something like: const MAGIC_LEN: usize = 12; const MAGIC: [u8; MAGIC_LEN] = *b"appendstore\n"; which will get a compilation error if they get out of sync. | |
| 36–40 | You could define a little FileState structure. | |
| 66–75 | Hm, I'd implement the "open tries RW then falls back to RO" explicitly as: match open_rw(...) {
Ok(file) =>...,
Err(err) if err == EACCESS => open_ro(...),
Err(err) => return Err(err),
}even if you don't need it externally. | |
Just curious: what's the difference between description() and display() methods?