Simple performance improvements for ldm

[mpu]

Description

This PR contains some simple performance improvements to the ldm matcher, together with some other cosmetic changes that I made "en passant" (variable renames, typo fixes, ...).

The changes to the ldm matcher only affect performance and the new code should produce precisely the same output as the baseline. Two changes helped improve the performance relatively uniformly:

• Instead of computing the tag with a branch and a fair amount of arithmetic operations in the hot loop of ZSTD_ldm_generateSequences we now compute a tag mask once before entering the loop and use it to check if the rolling hash satisfies the "tag condition". The tag mask is computed in such a way that the new tag condition is equivalent to the original one.
• When the tag condition is met but we did not find a match in the ldm hash table (it is the common case), we insert a new entry in the hash table using ZSTD_ldm_insertEntry instead of ZSTD_ldm_makeEntryAndInsertByTag which performs some redundant checks.

Benchmarks

The following files were used to assess the impact of the change:

• hhvm-rt.tar (5.9G) - an image of production version of HHVM
• l1m.tar (2.0G) - two snapshots of the linux kernel source code taken a month apart
• l1y.tar (1.9G) - two snapshots of the linux kernel source code taken a year apart
• l5.tar (544M) - the first 544M of an archive containing the linux kernel source code

The results are compiled in the table below. All times are computed as the best over a set of 5 runs.

FILE	CONFIG	DEFLATE Δ	TIME Δ
data/hhvm-rt.tar	--long=27 -1	=	- 07.69%
data/l1m.tar	--long=27 -1	=	- 07.15%
data/l1y.tar	--long=27 -1	=	- 05.76%
data/l5.tar	--long=27 -1	=	- 01.63%
data/hhvm-rt.tar	--long=30 -1	=	- 05.23%
data/l1m.tar	--long=30 -1	=	- 05.97%
data/l1y.tar	--long=30 -1	=	- 07.31%
data/l5.tar	--long=30 -1	=	- 04.13%
data/hhvm-rt.tar	--long=27 -8	=	- 07.26%
data/l1m.tar	--long=27 -8	=	- 10.21%
data/l1y.tar	--long=27 -8	=	- 05.42%
data/l5.tar	--long=27 -8	=	- 06.97%
data/hhvm-rt.tar	--long=30 -8	=	- 04.97%
data/l1m.tar	--long=30 -8	=	- 06.05%
data/l1y.tar	--long=30 -8	=	- 09.55%
data/l5.tar	--long=30 -8	=	- 01.70%
data/hhvm-rt.tar	--long=27 -16	=	+ 01.20%
data/l1m.tar	--long=27 -16	=	- 00.55%
data/l1y.tar	--long=27 -16	=	+ 06.68%
data/l5.tar	--long=27 -16	=	+ 01.80%
data/hhvm-rt.tar	--long=30 -16	=	+ 02.35%
data/l1m.tar	--long=30 -16	=	+ 02.68%
data/l1y.tar	--long=30 -16	=	+ 01.26%
data/l5.tar	--long=30 -16	=	+ 00.35%
data/hhvm-rt.tar	--long=27 -19	=	- 00.09%
data/l1m.tar	--long=27 -19	=	+ 01.43%
data/l1y.tar	--long=27 -19	=	- 00.52%
data/l5.tar	--long=27 -19	=	+ 02.94%
data/hhvm-rt.tar	--long=30 -19	=	+ 02.88%
data/l1m.tar	--long=30 -19	=	- 00.51%
data/l1y.tar	--long=30 -19	=	+ 01.14%
data/l5.tar	--long=30 -19	=	- 02.13%

[senhuang42]

nit: I believe the right hand side needs an explicit cast to appease the warning

[mpu]

Thanks for the hint. This should be fixed by my latest commit.

[senhuang42]

Thanks for the improvements, this generally looks pretty good to me!

The circle CI failure can probably get fixed with a rebase on top of the latest dev branch.

[terrelln]

I've measured a 2.5% - 4.5% speed improvement on several different files, which is in line with the measurements you've provided.

There are remaining calls to ZSTD_ldm_makeEntryAndInsertByTag() when we do find a match, and in ZSTD_ldm_fillLdmHashTable(). Do you think that it would speed up LDM to replace it with ZSTD_ldm_insertEntry() in ZSTD_ldm_fillLdmHashTable()?

[terrelln]

We also insert when we find an entry right? Could we use ZSTD_ldm_insertEntry() in that case too?

[mpu]

Yes, I thought it'd not bring much of a win but for the sake of consistency I updated it.

[mpu]

Thanks for your comments! I rebased on top of a recent dev branch and added one commit to address the feedback.
The performance characteristics should not have changed much. Nonetheless, I will run a final benchmark to make sure that is the case.

Do you think that it would speed up LDM to replace it with ZSTD_ldm_insertEntry() in ZSTD_ldm_fillLdmHashTable()?

In fillLdmHashTable the calls to makeEntryAndInsertByTag are not doing any superfluous work and likely get inlined, so I do not think we would gain anything by removing them.

[terrelln]

This looks great! I just have one minor nit about a renaming and it looks good to me.

[terrelln]

nit: this is not an offset it is an "index". Either keep curr or use currentIndex (note: current cannot be used because it conflicts with a macro in the Linux kernel).

I know that it is called offset in the ldmEntry_t struct, but that is a bad name. It should be renamed to index. If you want to include that renaming in this diff or a followup that would be great.

[mpu]

I renamed the variable to currentIndex. I also quickly re-run parts of the benchmarks on a relatively small but stable machine and got the following encouraging results.

FILE	CONFIG	DEFLATE Δ	TIME Δ
data/l5.tar	--long=27 -1	=	- 07.23%
data/l5.tar	--long=27 -8	=	- 01.65%
data/l5.tar	--long=27 -16	=	- 00.80%
data/l5.tar	--long=27 -19	=	+ 00.12%
data/l5.tar	--long=30 -1	=	- 09.20%
data/l5.tar	--long=30 -8	=	+ 00.24%
data/l5.tar	--long=30 -16	=	- 00.78%
data/l5.tar	--long=30 -19	=	+ 00.17%

[terrelln]

Awesome, thanks for the PR! Looking forward to the next one :)

[Cyan4973]

Thanks @mpu !

I tested your PR on a (fairly stable) desktop-class workstation,
and measured an ~8-9% improvement to LDM top speed (combined with multithreaded (low levels <=4) compression)
in line with this PR announce.