package/libs/zlib: Tidy up and add optimization #1329
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Compile/Runtime tested: mvebu, Linksys WRT3200ACM, LEDE trunk |
diizzyy
changed the title
|
I've only enabled this for NEON (32-bit) capable platforms as it's the only platforms I can test it on. |
|
I've given this a short build- and runtime test on:
No issues found (NEON actually gets enabled in the arm_cortex-a15_neon-vfpv4 binaries, it's mostly a no-op for mips, of course), the systems boot and work as expected; I haven't done any performance tests though. |
hauke
reviewed
Sep 16, 2017
| + state->havedict = 1; | ||
| + Tracev((stderr, "inflate: dictionary set\n")); | ||
| + return Z_OK; | ||
|
|
There was a problem hiding this comment.
could you put this in separate patches please
There was a problem hiding this comment.
You want me to create an additional patch?
This is in the original pull request by Simon Hoise but if you want me to split it sure.
| @@ -0,0 +1,100 @@ | |||
| diff --git a/CMakeLists.txt b/CMakeLists.txt | |||
| index 8e75f66..24d7329 100644 | |||
There was a problem hiding this comment.
could you add the source of this patch as a comment please.
There was a problem hiding this comment.
There's no upstream source, I created it myself.
package/libs/zlib/Makefile
Outdated
| @@ -48,6 +48,11 @@ endef | |||
|
|
|||
| TARGET_CFLAGS += $(FPIC) | |||
|
|
|||
| ifneq ($(findstring neon,$(CONFIG_TARGET_OPTIMIZATION)),) | |||
| CMAKE_OPTIONS += \ | |||
| -DARMv8=ON | |||
There was a problem hiding this comment.
Neon is also supported for ARMv7 CPUs.
Is it ok to activate the ARMv8 option on ARMv7 CPUs?
There was a problem hiding this comment.
Yes, it's safe.
madler/zlib#251 (comment)
|
@hauke |
|
Doing some quick benchmarks shows that compression level 9 gets about 10% faster, level 3 performs the same and so does deflate using minigzip64 (included as a test/sample app in zlib source code). |
|
@yousong |
|
@diizzyy - Could you break this up into 2 separate PRs. I see good value with the Makefile build cleanup. I don't have a strong preference on the minor ARM performance tweaks. |
yousong
reviewed
Nov 1, 2017
| @@ -74,7 +56,7 @@ define Build/InstallDev | |||
| $(CP) $(PKG_INSTALL_DIR)/usr/lib/libz.{a,so*} \ | |||
| $(1)/usr/lib/ | |||
| mkdir -p $(1)/usr/lib/pkgconfig | |||
| $(CP) $(PKG_INSTALL_DIR)/usr/lib/pkgconfig/zlib.pc \ | |||
| $(CP) $(PKG_INSTALL_DIR)/usr/share/pkgconfig/zlib.pc \ | |||
There was a problem hiding this comment.
Can you elaborate on this change? pkgconfig files are normally put in /usr/lib/pkgconfig.
There was a problem hiding this comment.
cmake defaults to share instead of lib
There was a problem hiding this comment.
I see. It's written in the CMakeLists.txt file. Thanks
|
Looks like chromium has already merged the optimisation in their local copy of zlib, so ACK for both patches from me. Thanks for the efforts. |
|
Ping |
|
Ping again |
|
Would using https://github.com/Dead2/zlib-ng.git as an alternative to patching in the fixes be possible? I just compiled with it and it seemed to work. It includes not only the optimizations from this PR, but apparently others. |
|
Unfortunately it doesn't support all platforms LEDE supports such as mips. |
|
Oh I see. Does LEDE build system have the ability to specify a different package to satisfy a dependency like zlib but only for certain architecture? Sorry I am very new to this. |
diizzyy
changed the title
Use build logic provided by toolchain instead of doing it manually. Signed-off-by: Daniel Engberg <daniel.engberg.lists@pyret.net>
This adds two optimizations for ARM: NEON optimized Adler(-)32 checksum algorithm (ARMv7 and newer NEON CPUs) ARM(v7+) specific optimization for inflate I've also connected inflate optimization to the build using the following source as template. mirror/chromium@0397489#diff-a62ad2db6c83dbc205d34bb9a8884f16 Additional info: https://codereview.chromium.org/2676493007/ https://codereview.chromium.org/2722063002/ Sources: madler/zlib#251 (only the first commit) madler/zlib#256 Signed-off-by: Daniel Engberg <daniel.engberg.lists@pyret.net>
diizzyy
changed the title
|
@pprindeville |
| endef | ||
|
|
||
| TARGET_CFLAGS += $(FPIC) |
There was a problem hiding this comment.
It originates from https://github.com/lede-project/source/blob/master/package/libs/zlib/Makefile#L52
Turning to Google for guidance I found several occurrences about this:
https://bugzilla.redhat.com/show_bug.cgi?id=832545
There was a problem hiding this comment.
I see that other package Makefile's have it... so it's nothing out of the ordinary.
Was just wondering if it was related or a target-of-opportunity while you were cleaning things up.
| @@ -46,27 +49,21 @@ define Package/zlib-dev/description | |||
| This package includes the development support files. | |||
| endef | |||
|
|
|||
| CONFIGURE_VARS := \ | |||
There was a problem hiding this comment.
And we don't need CONFIGURE_VARS or CONFIGURE_ARGS either?
There was a problem hiding this comment.
All needed information is provided by the toolchain or at least neither myself or pkgadd has managed to trigger something that fails.
hauke
approved these changes
Dec 11, 2017
package/libs/zlib/Config.in
Outdated
|
|
||
| config ZLIB_OPTIMIZE_SPEED | ||
| bool "Optimize for speed" | ||
| default n |
There was a problem hiding this comment.
n is already default no need to specify this.
|
@hauke |
Add option to use O3 optimization as not all devices have space constraints. This option is default using GCC in upstream but isn't in the CMake makefile for some reason. Source: https://github.com/madler/zlib/blob/master/configure#L170 Signed-off-by: Daniel Engberg <daniel.engberg.lists@pyret.net>
|
Updated so it strips any set -O* flag |
|
@diizzyy i've validated cmake works without it ; that seems to be needed only for the old i need the and it also seems that Python/Python3 does not like it if the |
|
@commodo |
|
yep ; just those 2 lines are needed for host-build + cmake to work ; thanks :) |
Some packages such as Python/Python3 (host pip/pip3) needs this to compile. More detailed explanation provided by Alexandru: "i need the zlib/host for Python/Python3 ; because, it seems the host pip/pip3 needs this to work ; i suspect in older versions this worked, because some of the host's build env would be used in the build, and then the zlib-dev from the host distro would be used ; now, the host-build does not seem to have any -I/usr/include stuff, which is good and it also seems that Python/Python3 does not like it if the zlib-dev package is too old, so using this zlib/host would be good for this as well" Source: #1329 (comment) Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> Signed-off-by: Daniel Engberg <daniel.engberg.lists@pyret.net>
|
@commodo |
commodo
reviewed
Dec 12, 2017
| @@ -96,5 +97,6 @@ define Package/zlib-dev/install | |||
| $(1)/usr/lib/pkgconfig/ | |||
| endef | |||
|
|
|||
| $(eval $(call HostBuild)) | |||
commodo
added a commit
to commodo/packages
that referenced
this pull request
Dec 13, 2017
This should fix the zlibmodule build on the host side. Usually, if zlib is not found, Python/Python3 builds fine without it, but there are some cases where the Python/Python3 interpreter on the host-side requires zlib to run. At the moment, zlib does not have a host-build. This should be available when this PR gets merged: lede-project/source#1329 [ or a similar one that contains host-build support for zlib ]. In the meantime, this change can go into Python/Python3. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
|
ping; is this ready to be merged as is ? or would it be easier to split up this PR into 2 parts [1 for the Makefile changes & 1 for the optimization patches] ? i'm interested in the HostBuild changes to be able to fix this, the Python/Python3 host-builds: |
|
oh... those links just vanished / are being regenerated.... |
|
This does not compile for me on a cortex a15: This is my config: |
|
The fpu feature is not set for this target, but it has a fpu, I am just compiling this toolchain again to see if this was the problem. |
jow-
pushed a commit
that referenced
this pull request
Jan 2, 2018
Some packages such as Python/Python3 (host pip/pip3) needs this to compile. More detailed explanation provided by Alexandru: "i need the zlib/host for Python/Python3 ; because, it seems the host pip/pip3 needs this to work ; i suspect in older versions this worked, because some of the host's build env would be used in the build, and then the zlib-dev from the host distro would be used ; now, the host-build does not seem to have any -I/usr/include stuff, which is good and it also seems that Python/Python3 does not like it if the zlib-dev package is too old, so using this zlib/host would be good for this as well" Source: #1329 (comment) Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> Signed-off-by: Daniel Engberg <daniel.engberg.lists@pyret.net>
|
Thank you for the patch it was applied to master. |
SpiralP
pushed a commit
to SpiralP/lede-source
that referenced
this pull request
Jan 2, 2018
Some packages such as Python/Python3 (host pip/pip3) needs this to compile. More detailed explanation provided by Alexandru: "i need the zlib/host for Python/Python3 ; because, it seems the host pip/pip3 needs this to work ; i suspect in older versions this worked, because some of the host's build env would be used in the build, and then the zlib-dev from the host distro would be used ; now, the host-build does not seem to have any -I/usr/include stuff, which is good and it also seems that Python/Python3 does not like it if the zlib-dev package is too old, so using this zlib/host would be good for this as well" Source: lede-project#1329 (comment) Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> Signed-off-by: Daniel Engberg <daniel.engberg.lists@pyret.net>
salzmdan
pushed a commit
to salzmdan/packages
that referenced
this pull request
Jan 8, 2018
This should fix the zlibmodule build on the host side. Usually, if zlib is not found, Python/Python3 builds fine without it, but there are some cases where the Python/Python3 interpreter on the host-side requires zlib to run. At the moment, zlib does not have a host-build. This should be available when this PR gets merged: lede-project/source#1329 [ or a similar one that contains host-build support for zlib ]. In the meantime, this change can go into Python/Python3. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
jollaman999
pushed a commit
to jollaman999/openwrt
that referenced
this pull request
Jan 13, 2018
Some packages such as Python/Python3 (host pip/pip3) needs this to compile. More detailed explanation provided by Alexandru: "i need the zlib/host for Python/Python3 ; because, it seems the host pip/pip3 needs this to work ; i suspect in older versions this worked, because some of the host's build env would be used in the build, and then the zlib-dev from the host distro would be used ; now, the host-build does not seem to have any -I/usr/include stuff, which is good and it also seems that Python/Python3 does not like it if the zlib-dev package is too old, so using this zlib/host would be good for this as well" Source: lede-project/source#1329 (comment) Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com> Signed-off-by: Daniel Engberg <daniel.engberg.lists@pyret.net>
jow-
pushed a commit
to jow-/packages
that referenced
this pull request
Jan 15, 2018
This should fix the zlibmodule build on the host side. Usually, if zlib is not found, Python/Python3 builds fine without it, but there are some cases where the Python/Python3 interpreter on the host-side requires zlib to run. At the moment, zlib does not have a host-build. This should be available when this PR gets merged: lede-project/source#1329 [ or a similar one that contains host-build support for zlib ]. In the meantime, this change can go into Python/Python3. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
lynxis
pushed a commit
to lynxis/packages
that referenced
this pull request
Jan 3, 2019
This should fix the zlibmodule build on the host side. Usually, if zlib is not found, Python/Python3 builds fine without it, but there are some cases where the Python/Python3 interpreter on the host-side requires zlib to run. At the moment, zlib does not have a host-build. This should be available when this PR gets merged: lede-project/source#1329 [ or a similar one that contains host-build support for zlib ]. In the meantime, this change can go into Python/Python3. Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use build logic provided by toolchain instead of doing it manually and add some optimization.
Signed-off-by: Daniel Engberg daniel.engberg.lists@pyret.net