Enable tcmalloc by default. Requires gperftools-libs
This is for 1.3.6 (RHEL 7.4)
Metadata Update from @mreynolds: - Issue assigned to mreynolds
Metadata Update from @mreynolds: - Issue priority set to: 3 - Issue set to the milestone: 1.3.6.0
Metadata Update from @mreynolds: - Custom field origin adjusted to Community - Custom field type adjusted to enhancement - Custom field version adjusted to 1.3.6
Metadata Update from @mreynolds: - Issue tagged with: Performance
Adding patch
<img alt="0001-Issue-49141-Use-tcmalloc-by-default.patch" src="/389-ds-base/issue/raw/files/c9408ed955095c61a2c3daa65f673eac59a67047d48e6e55d7e4b5193656aaed-0001-Issue-49141-Use-tcmalloc-by-default.patch" />
Metadata Update from @mreynolds: - Custom field reviewstatus adjusted to review
Hi Mark,
2 questions for the spec file... 1) Isn't it safer to "Require" the library even if it could be installed by default? Requires: gperftools-libs 2) It might be useful to define the global variable %global use_tcmalloc 1 and put the enabling code in %if %{use_tcmalloc} ?
Thanks!
Hi Mark, 2 questions for the spec file... 1) Isn't it safer to "Require" the library even if it could be installed by default? Requires: gperftools-libs
But it is required to do the "build" which is why I used BuildRequires. As long as "Requires" is sufficient I'll change it.
2) It might be useful to define the global variable %global use_tcmalloc 1 and put the enabling code in %if %{use_tcmalloc} ?
Yeah I thought about that, but eventually decided against it. I have no problem adding it though :)
I'll start on a new patch...
Oh, if you have a reason to drop it, I respect it. They are my "questions". :)
Hi Mark, 2 questions for the spec file... 1) Isn't it safer to "Require" the library even if it could be installed by default? Requires: gperftools-libs But it is required to do the "build" which is why I used BuildRequires. As long as "Requires" is sufficient I'll change it. I think we need both?
But it is required to do the "build" which is why I used BuildRequires. As long as "Requires" is sufficient I'll change it. I think we need both?
Revised patch:
<img alt="0001-Issue-49141-Use-tcmalloc-by-default.patch" src="/389-ds-base/issue/raw/files/afef0b8cba2a0ba582c94a7d052f1976e0cc0ad3e481f54da7502731c0c0722f-0001-Issue-49141-Use-tcmalloc-by-default.patch" />
Sorry, Mark. My comment was not clear...
I meant to add "Requires: gperftools-libs" for the installation. 148 +%if %{use_tcmalloc} 149 +BuildRequires: gperftools-devel 1?? +Requires: gperftools-libs 150 +TCMALLOC_FLAGS="--with-tcmalloc" 151 +%endif
Well I add the global flag for tcmalloc in the spec file. However, I'm still unsure about BuilldRequires vs Requires. When I said that it was required to build the server it was because "make install" fails if libtcmalloc was not present on the system. But it is also definitely needed for the server after compiling.
---> I just saw your comment about Requires. Okay - that makes sense. I'll work on a new patch :)
New patch
<img alt="0001-Issue-49141-Use-tcmalloc-by-default.patch" src="/389-ds-base/issue/raw/files/4f40950e40ff7bae8af06d42e4f24351e332946c032c1c456e58656d178c58de-0001-Issue-49141-Use-tcmalloc-by-default.patch" />
Thank you, Mark! You have my ack.
Metadata Update from @nhosoi: - Custom field reviewstatus adjusted to ack (was: review)
1c790df..0349d01 master -> master commit 0349d01 Author: Mark Reynolds mreynolds@redhat.com Date: Tue Feb 21 13:05:21 2017 -0500
Metadata Update from @mreynolds: - Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1186512
Design doc:
http://www.port389.org/docs/389ds/design/tcmalloc-design.html
This fixes an error in the spec file:
0349d01..82b8f9a master -> master
Metadata Update from @mreynolds: - Issue close_status updated to: fixed - Issue status updated to: Closed (was: Open)
Metadata Update from @mreynolds: - Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1425837 (was: https://bugzilla.redhat.com/show_bug.cgi?id=1186512)
<img alt="0004-Ticket-49141-Enable-tcmalloc.patch" src="/389-ds-base/issue/raw/files/51c19f5bdd05f0cbeaed6b51106b28b45ec0af6c02dfe000a30b8dd610bca160-0004-Ticket-49141-Enable-tcmalloc.patch" />
This fixes some issues with tcmalloc and the new NS bundling. Additionally, this resolves an issue with tcmalloc with ASAN, and tcmalloc on s390.
Metadata Update from @firstyear: - Issue status updated to: Open (was: Closed)
Metadata Update from @firstyear: - Custom field reviewstatus adjusted to review (was: ack)
commit f9351cf To ssh://git@pagure.io/389-ds-base.git a95889d..f9351cf master -> master
Metadata Update from @firstyear: - Issue close_status updated to: fixed - Issue status updated to: Closed (was: Open)
JFTR, acked after successful build in brew on all arches and discussion on IRC.
Metadata Update from @vashirov: - Custom field reviewstatus adjusted to ack (was: review)
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/2200
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: fixed)
Login to comment on this ticket.