Page MenuHomeFreeBSD

remove duplicate lz4 implementations
ClosedPublic

Authored by tsoome on Oct 15 2019, 9:05 AM.

Details

Reviewers
None
Group Reviewers
bhyve
Commits
rS354253: Remove duplicate lz4 implementations
Summary

Port illumos change: https://www.illumos.org/issues/11667

Move lz4.c out of zfs tree to opensolaris/common/lz4, adjust it to be
usable from kernel/stand/userland builds, so we can use just one single
source. Add lz4.h to declare lz4_compress() and lz4_decompress().

Test Plan

build/install/boot using lz4 with zfs.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tsoome created this revision.Oct 15 2019, 9:05 AM

I like the direction in general.

By the way, (unrelated to this change itself), have you considered updating lz4 code with the upstream ( https://github.com/lz4/lz4 )? I'm mostly concerned about getting more diverged from the upstream would make it harder for us to re-sync the code in the future, especially when we have already missed some of the optimization that happened upstream. There are also some other interesting changes upstream, for example, it used a set of macros to avoid downstream consumers to make changes like #ifdef _KERNEL in the code that would spread all over places, and that would make it easier for future upgrade too.

The change looks good to me as-is, but please consider these comments.

sys/cddl/boot/zfs/zfssubr.c
164 ↗(On Diff #63287)

Note that the boot/ copy of lz4 was stripped down and #included to give the compiler to liberty to optimize away unused code. This practice is probably no longer relevant today (lz4 footprint is small anyways), but I'd suggest comparing the size before and after this change.

sys/cddl/contrib/opensolaris/common/lz4/lz4.h
2 ↗(On Diff #63287)

Could you please consider licensing this file under the same license that LZ4 uses?

tsoome updated this revision to Diff 63511.Mon, Oct 21, 10:20 AM

update the header for lz4.h, rebase on latest current.

I like the direction in general.
By the way, (unrelated to this change itself), have you considered updating lz4 code with the upstream ( https://github.com/lz4/lz4 )? I'm mostly concerned about getting more diverged from the upstream would make it harder for us to re-sync the code in the future, especially when we have already missed some of the optimization that happened upstream. There are also some other interesting changes upstream, for example, it used a set of macros to avoid downstream consumers to make changes like #ifdef _KERNEL in the code that would spread all over places, and that would make it easier for future upgrade too.
The change looks good to me as-is, but please consider these comments.

I do understand the temptation, but I rather keep the current code as is for this change. The lz4 update to more recent source will need a lot of testing and I do not have time to do that - not now at least. Here we are operating on the current source and we only do apply syntactic sugar (so to speak) to keep compilers happy.

tsoome marked an inline comment as done.Mon, Oct 21, 10:40 AM
tsoome added inline comments.
sys/cddl/boot/zfs/zfssubr.c
164 ↗(On Diff #63287)

We do use function-sections and data sections with build and let ld to strip away the unused sections, the bios version is losing the compression bits this way. The UEFI version does not get clean - there we do build shared library, and I havent figured out exact steps how to reduce things like lz4_compress. I think we would need to get those symbols demoted to local via mapfile, then ld can strip them off.

This revision was not accepted when it landed; it landed in state Needs Review.Sat, Nov 2, 12:28 PM
This revision was automatically updated to reflect the committed changes.