search

vasi / squashfuse

FUSE filesystem to mount squashfs archives
Other
288 stars 66 forks source link

Fb performance improvements #38

Closed kevin-vigor closed 3 years ago

kevin-vigor commented 4 years ago

David, here is the cleaned up patch series.

The most interesting bit is the multithreaded decompression, but other small performance enhancements are included.

Note the addition of some simple test cases, which are run with 'make check'. For maximum test coverage, the fio and gtest-devel packages should be installed, but even without, a basic smoke test should execute successfully.

Multithreaded mode has to be explicitly enabled at configure time, and I believe the autoconf stuff is such that the default (singlethreaded) build should still work on any system that could previously build. I have only tested on linux, though.

vasi commented 4 years ago

Doesn't configure on macOS: configure: error: conditional "am__fastdepCXX" was never defined. Never seen that one before! Looks like cuz of the conditional AC_PROG_CXX.

I'll try and take these one commit at a time.

kevin-vigor commented 4 years ago

Ugh, sorry, I have no access to a macos box at this point :(

On Sun, Feb 9, 2020 at 6:57 PM Dave Vasilevsky notifications@github.com wrote:

Doesn't configure on macOS: configure: error: conditional "am__fastdepCXX" was never defined. Never seen that one before! Looks like cuz of the conditional AC_PROG_CXX.

I'll try and take these one commit at a time.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/vasi/squashfuse/pull/38?email_source=notifications&email_token=ACYGT5G5WIJBYEPAEJONZV3RCCYATA5CNFSM4KREHUIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELG744I#issuecomment-583925361, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACYGT5B47XHTN5RJVXKGVVDRCCYATANCNFSM4KREHUIA .

vasi commented 4 years ago

Ok, first commit seems alright! A bit hard to run the tests—I didn't have anything handy with fuse3, and squashfs-tools with zstd support. I guess we could test with different compressors and older fusermount, but no urgency.

I'll review more tomorrow.

vasi commented 4 years ago

Ok, pulled out the smoke tests into #39 . Let me know if that looks ok, and then we can try moving on to more commits!

r0l1 commented 4 years ago

Any progress on getting this merged?

keichi commented 3 years ago

It would be great if this could get merged. My workload performs many concurrent reads and shows poor performance using squashfuse compared to the kernel driver.

haampie commented 3 years ago

I'm also interested in this PR :)

chipturner commented 3 years ago

@kevin-vigor can you update this patch to fix merge conflicts and maybe pull in some of our other perf improvements?

vasi commented 3 years ago

I got the first couple of these commits merged, after some fixups so they compile properly on non-Linux platforms. I also setup CI for squashfuse, so hopefully it becomes easier to ensure we're good on all platforms.

I looked at the endianness commit, and the non-test code seems fine. Unfortunately gtest isn't a great fit:

kevin-vigor commented 3 years ago

I happen to have some time on my hands this week and access to a Mac, so I'll take a look at this tomorrow.

@vasi: do you have a test framework in mind? It's been some time since I looked at this but as I recall the interesting (integration) tests are shell scripts (which are almost certainly going to be a pain to port to anything non-linux).

vasi commented 3 years ago

The existing shell tests currently work fine on OSX, FreeBSD and Windows. I've been fixing them as we go, see things like sq_umount and sq_md5sum. They're even running in CI, so if something isn't working, we'll find out fast.

It's just the C tests (like endiantest.cpp) where I'd like to switch to something lightweight. Looks like check exists on just about every platform? Unity and CuTest are also very small.

kevin-vigor commented 3 years ago

FYI I rebased, removed google test, and split this stack of diffs up. New pull requests submitted, this request should be abandoned.