[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: How to go forward with zstd
From:       Norbert Lange <nolange79 () gmail ! com>
Date:       2021-12-13 23:23:27
Message-ID: CADYdroMMLDGAn-qz96jaJ7PFoX5QHTeoco442__6dWGpCkD5kg () mail ! gmail ! com
[Download RAW message or body]

Am Mo., 13. Dez. 2021 um 18:36 Uhr schrieb Denys Vlasenko
<vda.linux@googlemail.com>:
>
> On Fri, Dec 10, 2021 at 10:17 PM Norbert Lange <nolange79@gmail.com> wrote:
> > Hello,
> >
> > the question is still open whether BB is ok with taking the upstream
> > zstd sources and semi-automatically export them [1]. cost is around
> > 17K in binary size with a few manual code removals, ie- without
> > diverting from upstream too much. That code is also available on
> > github [2]
> >
> > upstream would be willing to support continuous testing, should we
> > have some modifications that can end up there (ie. separate codepaths
> > guarded by macros). So probably that would mean getting the sources
> > exported *fully* automated, then check if those compile and work with
> > some testing stubs. [3]
>
> Well... code as it stands now is not written with much thought
> about keeping size smaller. I can easily find places
> which I'm not happy about. Look at this:

Yeah, I just wonder whether you thing its feasible change upstream so
that you are
satisfied. It is a different project, different goals,
it will never be as small as code written from ground up to be small.
Some unhappy places would live in a subfolder, always as kinda alien code.

How far do you want to change it?

>
> zstd_internal.h
>
> static UNUSED_ATTR const U32 LL_bits[MaxLL+1] = {
>      0, 0, 0, 0, 0, 0, 0, 0,
>      0, 0, 0, 0, 0, 0, 0, 0,
>      1, 1, 1, 1, 2, 2, 3, 3,
>      4, 6, 7, 8, 9,10,11,12,
>     13,14,15,16
> };
> static UNUSED_ATTR const S16 LL_defaultNorm[MaxLL+1] = {
>      4, 3, 2, 2, 2, 2, 2, 2,
>      2, 2, 2, 2, 2, 1, 1, 1,
>      2, 2, 2, 2, 2, 2, 2, 2,
>      2, 3, 2, 1, 1, 1, 1, 1,
>     -1,-1,-1,-1
> };
>
> All "bits" arrays fit into 16 bit ints. All "defaultNorm"
> arrays fit into 8-bit signed ints.

Yeah, did some test, apparently possible to change all of those used
for decompression.
Gonna review this a bit more and send it upstream.

>
> Also, defining arrays in .h files is wrong.

pointer tricks with errno could be considered wrong ;)
My patch just includes all .h and .c files, so it makes no practical
difference for us.

>
> I bet with detailed review many more things like this
> are to be discovered.

Yeah, some things might be just because of different goals, ie.
giving up "small" for "fast". Or keeping the legacy decompressors alive.

> Anyone is willing to submit patches upstream
> until it looks better?

To an extend I am.

Norbert
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]