raft
raft copied to clipboard
canonical
Cross-platform support
@AshfordN The "My" prefix for disambiguation might be better as "Raft". Just a thought.
Would also like to get some darwin love in on these changes but I guess that can happen with a followup after this lands.
Codecov Report
Merging #119 into master will decrease coverage by
0.02%. The diff coverage is76.57%.
@@ Coverage Diff @@
## master #119 +/- ##
==========================================
- Coverage 86.1% 86.08% -0.03%
==========================================
Files 47 47
Lines 7436 6892 -544
Branches 1166 1142 -24
==========================================
- Hits 6403 5933 -470
+ Misses 1028 959 -69
+ Partials 5 0 -5
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/uv_os.c | 94.59% <ø> (-1.8%) |
:arrow_down: |
| src/byte.c | 98.54% <ø> (-0.04%) |
:arrow_down: |
| src/recv.c | 81.39% <100%> (+0.34%) |
:arrow_up: |
| src/uv_send.c | 92.66% <100%> (-0.17%) |
:arrow_down: |
| src/replication.c | 79.73% <100%> (+1.71%) |
:arrow_up: |
| src/heap.c | 100% <100%> (ø) |
:arrow_up: |
| src/recv_append_entries.c | 86% <100%> (-1.04%) |
:arrow_down: |
| src/uv_finalize.c | 67.14% <50%> (-14.2%) |
:arrow_down: |
| src/raft.c | 83.07% <50%> (+1.91%) |
:arrow_up: |
| src/uv_truncate.c | 75.9% <57.14%> (+1.99%) |
:arrow_up: |
| ... and 51 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update b1cd439...eff2176. Read the comment docs.
What's the status of this branch? Is it ready for review and possibly landing?
What's the status of this branch? Is it ready for review and possibly landing?
@AshfordN ? ^^^
Hey, I apologise for my inactivity on this branch but I've been a bit busy lately. At the moment, I can't seem to get this linked with libuv despite my best efforts. Although I must admit that I'm actually cross-compiling with MinGW on Linux; which might not be the ideal environment for the ground work that I'm doing. Aside from that, there were some low-level memory handling here that could not be ported 1-to-1 (see this article). I'd appreciate it if someone could reviewed what I have so far and make the necessary recommendations.
Hey, I apologise for my inactivity on this branch but I've been a bit busy lately.
No worries!
Aside from that, there were some low-level memory handling here that could not be ported 1-to-1 (see this article).
The article itself has this comment at the bottom:
The Universal C Runtime that is provided with Visual Studio via the Windows 10 SDK does not include support for aligned_alloc. Instead, _aligned_malloc and _aligned_free must be used instead.
So can't use use _aligned_malloc as documented here?
I'd appreciate it if someone could reviewed what I have so far and make the necessary recommendations.
Not sure I'll be able to provide much feedback, since I'm not familiar with Windows. From my point of view, as long as the code works on Windows (something which I trust you to test) and does not break Linux (something which will be tested automatically by Travis), I've no objection merging your work.
So can't you use _aligned_malloc as documented here?
It's not as straight forward. I made a short comment in my version of heap.c about this issue. Currently all memory is freed with a call to free(). However, memory allocated with _aligned_malloc() needs to be freed with _aligned_free(), so there needs to be extra bookkeeping in order to keep things safe, and I'm not quite sure how to attach that extra bookkeeping at such a low level.
I left it half-done for now since I didn't want to make major changes to the code structure.
I'll attempt to bring this to review stage sometime this week.
So can't you use _aligned_malloc as documented here?
It's not as straight forward. I made a short comment in my version of
heap.cabout this issue. Currently all memory is freed with a call tofree(). However, memory allocated with_aligned_malloc()needs to be freed with_aligned_free(), so there needs to be extra bookkeeping in order to keep things safe, and I'm not quite sure how to attach that extra bookkeeping at such a low level. I left it half-done for now since I didn't want to make major changes to the code structure. I'll attempt to bring this to review stage sometime this week.
Ah I see. There are very few cases were aligned_malloc is needed (basically just for direct I/O), it should not be too bad to special-case them and use a dedicated free().
Hey @AshfordN / @Kerollmops , what's the status of this PR?
As far as I can tell, with the introduction of raft_heap->aligned_free() and the internal posix_fallocate emulation code there road should now be paved for completing this work.
Hey, My main challenge with this is actually completing the build process. For some reason, MinGW isn't linking libuv and I'm not sure why. Nevertheless, I'm still able to complete the build process when targeting Linux, so I don't think I broke the build script. Work has been preventing me from pinning down the issue so I'd appreciate it if someone more familiar with windows could point out any shortcomings. As you said, everything seem to be in place to complete the work on this PR, but since I'm unable to test it, I can't say for sure.
@Kerollmops am I to understand that your branch is working on OSX?
@freeekanayaka also, does my fallback code here look sensible?
That might be enough in practice, although it does not completely match the current semantics: for example UvWriterClose currently waits for in-flight write requests to complete, before firing the given UvWriterCloseCb. Also, you'll need to put in place some more code to fire the UvWriterReqCb and UvWriterCloseCb.
Considering that the current implementation already has fallback support for performing the writes using libuv's threadpool (which is exactly how the uv_fs_write API you are using is implemented internally in libuv), then I'd suggest to disable the non-portable code (eventfd/io_submit) using #if/#else. You'll need to put in place more #if/#else dance, but you'll be able to leverage the existing fallback implementation without essentially re-inventing it.
As far as I can tell, with the introduction of raft_heap->aligned_free() and the internal posix_fallocate emulation code there road should now be paved for completing this work.
It seems to do the work but I didn't tried it, I didn't tried to create a wrapper of this API around the Rust Allocator API, sorry. I will neither be able to test that in a near futur, but in my understading this aligned_free function was the only missing one.
@Kerollmops am I to understand that your branch is working on OSX?
I wouldn't say that it works but at least it compiles and kind of run until it locks or something. No more messages are passed between peer. It is really hard to debug this kind of stuff and don't have much time to allocate to it :/
I wouldn't say that it works but at least it compiles and kind of run until it locks or something. No more messages are passed between peer.
This might be due to the questionable implementation of the fallback code discussed above. The fix might not be too difficult based on what @freeekanayaka said, but I'd have to figure out the build process so that I can at least test the code. Unfortunately I can't give a definite timeline for this PR, but I will try to complete the work.
Has there been any non-public work on this lately? While I don't have a lot of time, it would be cool to get this running on OSX. I have it compiled and kind-of working... but I definitely don't trust it.
Has there been any non-public work on this lately?
Not that I'm aware of.
Interesting PR, tempting to crack open my Windows development environment which has been lying dormant for over a year now. clang is also worth a consideration for windows, it compiles native Windows binaries now (though I think you still need to use a different linker, unless that's changed since I last looked at it). Although mingw is a nice alternative also, clang is more native. If you compile directly on Windows also, it might make it easier to run the test suite on Windows.
