Bug 170303 - Review Request: google-perftools: Very fast malloc & performance analysis tools
Summary: Review Request: google-perftools: Very fast malloc & performance analysis tools
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dmitry Butskoy
QA Contact: Fedora Package Reviews List
URL: http://code.google.com/p/google-perft...
Whiteboard:
: 458313 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-10-10 16:49 UTC by Tom "spot" Callaway
Modified: 2008-08-21 13:21 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-04-30 03:32:53 UTC
Type: ---
Embargoed:
dmitry: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)
make check from rawhide (142.65 KB, text/plain)
2006-01-05 15:39 UTC, Tom "spot" Callaway
no flags Details

Description Tom "spot" Callaway 2005-10-10 16:49:08 UTC
Spec Name or Url: http://www.auroralinux.org/people/spot/review/google-perftools.spec
SRPM Name or Url: http://www.auroralinux.org/people/spot/review/google-perftools-0.3-1.src.rpm
Description: 
Perf Tools is a collection of performance analysis tools, including a
high-performance multi-threaded malloc() implementation that works
particularly well with threads and STL, a thread-friendly heap-checker,
a heap profiler, and a cpu-profiler.

Comment 1 Dmitry Butskoy 2005-10-13 15:28:35 UTC
First of all, I'm a little confused with presence of the symbol "&" in the
Summary field. Sometime somewhere something will by all means spoiled ny it... :)



Comment 2 Dmitry Butskoy 2005-10-13 16:14:23 UTC
Remarks and nitpicks:
- Group "Applications/System" seems does not meet the purpose of this soft, IMHO
it should be "Development/Tools" or similar.
- The stuff with explicitly "%{_docdir}" etc. looks not very well. I would
suggest to remove all the docs in their default path (after "make install"), and
then to specify it explicitly as "%doc something" etc. in the %files section.
- It would be better to move all html documentation and related files into a
separate subdirectory (for example, "mv doc html" in "%install" section, then
specify "%doc html" in "%files" ...)
- A section "%check" must be used, as "make check" possibility is present.

- There is no license text as a separate file.
- There are two names "google-perftools" and "goog-perftools" (the last is used
in URLs). Why "goog" has appeared? I guess it is some trademark issues here.
Therefore the package probably should be renamed...


Comment 3 Tom "spot" Callaway 2005-10-13 18:09:15 UTC
Google is the upstream maintainer. goog-perftools is their sourceforge name, no
idea why they did it. The actual package name is "google-perftools", thus the
name is ok.

Anything in %{_docdir} is already %doc as far as rpm is concerned. This would be
extra steps to achieve the exact same result.

I don't see any sense in moving the html documentation into a "html" directory,
as all of the documentation is html or related images.

Having %check run make check doesn't help us, it fails because of lots of
missing debuginfo packages.

All other suggestions have been implemented in -2:

New SRPM:
http://www.auroralinux.org/people/spot/review/google-perftools-0.3-2.src.rpm

New SPEC:
http://www.auroralinux.org/people/spot/review/google-perftools.spec

Comment 4 Dmitry Butskoy 2005-10-14 11:04:55 UTC
> Google is the upstream maintainer. goog-perftools is their sourceforge name, no
> idea why they did it. The actual package name is "google-perftools", thus the
> name is ok.

I'm still confused by this "goog" ...

Whether there are any precedents of inclusion into Fedora of packages which
names contain trade marks?

IANAL, but in the current legal atmosphere it would be better to try to ask the
upstream about this issue. I.e., is such a name is allowed (I guess yes), and
why "goog" is on SourceForge. The answer can be included as a file into %doc,
and this will close the issue forever. The same time it would be possible to ask
them to include a text of its license in the upstream distribution, or just to
give a url link to it.


> Having %check run make check doesn't help us, it fails because of lots of
> missing debuginfo packages.

Not understood... WTF "debuginfo packages" ??..

"make check" actually does two things: "make $(check_SCRIPTS)" and "make
check_TESTS". check_TESTS passed OK, but $(check_SCRIPTS) failed. I'll try to
figure out now, why.

Anyway, this package contains a check stuff. It is included to make sure that
the compiled things work correctly. Now, "make check" fails, it mean that
results are incorrect.


Comment 5 Dmitry Butskoy 2005-10-14 11:47:00 UTC
"make check" issue:

First, that i've figure out, is uninitialized objects in the perl script
./src/pprof. BTW this script goes to /usr/bin "as is" without any changes...

Also I've found some bug report at upstream:
http://sourceforge.net/tracker/index.php?func=detail&aid=1291707&group_id=133355&atid=726859

All shows that this package is yet not stable enough...

Comment 6 Dmitry Butskoy 2005-10-14 12:59:54 UTC
When I compile with different optimization levels, "make check" fails in
different places.

"-O0 -DUSE_NO_SPINLOCKS" and "-O1" lead to:
ERROR: Leaks found in main heap check, aborting
./src/tests/heap-checker_unittest.sh: line 59: 31153 Aborted   $HEAP_CHECKER

"-O2" produces:
Check failed: check.SameHeap() == true

and "-O3" gives a lot of:
>>> Test failed for Allocate: didn't use 90% of cpu


IMHO, it is a situation for the technical knockout :-)



Comment 7 Dmitry Butskoy 2005-11-01 14:25:13 UTC
Tom,

new version 0.4 has appeared in upstream...



Comment 8 Dmitry Butskoy 2005-11-02 15:51:20 UTC
First, it is not compiled (at least under FC3) .

When it is compiled though (:), after a little patch), the check results seem to
be the same :(

Wait for 0.5 ...

Comment 9 Dmitry Butskoy 2005-11-23 17:03:01 UTC
0.5: all the same...

Tom,

I check this on FC3 box. Can you do:
  rpmbuild -bc google-perftools.spec
  cd  <builddir><buildsubdir>
  make check
under FC4 or rawhide box?

The issues at "make check" stage can be caused by "gcc/g++ < 4.0" at my FC3 box...

Comment 10 Tom "spot" Callaway 2006-01-05 15:38:44 UTC
Tested make check on rawhide, it doesn't work properly. One of the scripts
needed patching just to work, and then it threw an error. I suspect that this
application may rely on Linuxthreads, which are going away in development,
instad of NPTL.



Comment 11 Tom "spot" Callaway 2006-01-05 15:39:55 UTC
Created attachment 122826 [details]
make check from rawhide

Comment 12 Dmitry Butskoy 2006-01-23 12:40:14 UTC
> I suspect that this application may rely on Linuxthreads, which are going away
> in development, instad of NPTL.

I've tried "make check" with "LD_ASSUME_KERNEL=2.2.9" (/lib/libc.so) and with
"LD_ASSUME_KERNEL=2.4.9" (/lib/i686/libc.so). Both was failed as well as with
/lib/tls/libc.so . In different places...

Now we see that the "make check" result depends on the compiler optimisation
options and also depends on the LD_ASSUME_KERNEL variants. All the results are
failure in the different places.


Maybe email upstream about these issues?


Comment 13 Dmitry Butskoy 2006-02-06 12:39:25 UTC
0.6 upstream.

All the same... :(

Comment 14 Edward A. Falk 2006-02-07 01:48:13 UTC
Hi all; a few answers to questions:

The package we built on sourceforge was named "google-perftools".  You should
stick to that naming convention to avoid confusion and dependency problems.  You
should definately avoid "perftools" because there are other software packages by
that name.

The web page on sourceforge is named "goog-perftools", but that's not
significant.  All of our sourceforge projects start with "goog-".

Version 0.6 of perftools is now on sourceforge, and it removes the "docdir" cruft.

I'll be taking a look at your revised packages and possibly incorporating them
into our version.

Packaging questions can be directed to me at efalk.net.  Bug
reports should be filed at http://sourceforge.net/projects/goog-perftools/

General questions can be directed to opensource at google.com; we all read that one.

  -ed falk, efalk.net

Comment 15 Edward A. Falk 2006-02-07 01:52:34 UTC
Oh, and if 0.6 is still giving you some sort of build or check problems, please
file a bug at the sourceforge web page, include the errors you're getting.

  -ef

Comment 16 Tom "spot" Callaway 2006-02-07 16:19:44 UTC
Upstream bug filed:
http://sourceforge.net/tracker/index.php?func=detail&aid=1426205&group_id=133355&atid=726859

(it got filed twice because sourceforge is a laggy piece of crap, but I closed
the dupe)

Comment 17 Craig Silverstein 2006-02-08 22:38:28 UTC
Another note from an 'upstream' person: thanks for filing the
heap-checker_unittest failure on sourceforge.  I'll be following up with it there.

As to the other bugs reported on this page, I tried compiling
google-perftools-0.6 on a standard FC3 machine and could not reproduce the
problems reported.  The code compiled straight out of the box, and pprof did not
give any syntax errors or warnings.  If you do not see the same, please file bug
reports on sourceforge with details of how to reproduce the error, and we'll
look into fixing them.

craig

Comment 18 Dmitry Butskoy 2006-02-09 13:55:45 UTC
> I tried compiling google-perftools-0.6 on a standard FC3 machine and could not
> reproduce the problems reported.

Well, I use FC3 too (with all the updates needed).

To reproduce:
1. Take the source rpm here:
http://dmitry.butskoy.name/google-perftools-0.6-1.src.rpm
It is Tom's srpm with just the tarball upgraded.

2. rpm -i *.src.rpm

3. Go to your SPEC directory and run "rpmbuild -bc"
(i.e., compile only)

4. Go to the appropriate BUILD subdirectory and run "make check"

You should see the failure (and a lot of another falusers when playing with
various optimisation levels and LD_ASSUME_KERNEL's ...)




Comment 19 Craig Silverstein 2006-02-14 00:12:09 UTC
We've been doing some checking here, and it appears the problem has to do with
kernel skew.  The heap-checker code, in particular, has to do some pretty hacky
stuff to deal with thread-local storage, etc -- implementations that change
slightly from kernel to kernel.

We're looking at fixing the code so it works with the kernels in FC3/4/5, but it
may take a little while.

craig

Comment 20 Craig Silverstein 2006-03-09 10:07:03 UTC
Just wanted to give an update.  We've found other issues with recent kernels,
notably in changes to whether threads are allowed to PTRACE_ATTACH to sibling
threads.  These require a significant rewrite to the heap-checker code.  We're
working on it!  But we still have a ways to go until it will be all ready.

Comment 21 Tom "spot" Callaway 2006-03-09 14:17:21 UTC
Awesome, we'll be patiently waiting for it. :)

Comment 22 Craig Silverstein 2006-04-22 02:23:47 UTC
OK, we've finally figured out all the wrinkles.  google-perftools 0.7 was just
released on sourceforge, and I've confirmed it compiles and passes 'make check'
on my fc3 system, at least.  Have at it!

http://sourceforge.net/project/showfiles.php?group_id=133355

Comment 23 Tom "spot" Callaway 2006-04-22 16:41:33 UTC
Fails make check on FC-5 (i386):

>>> TESTING ./heap-checker_unittest with HEAPPROFILE= and HEAPCHECK=
In main()
HeapChecker: Heap checker is not active, hence checker "all" will do nothing!
HeapChecker: To activate set the HEAPCHECK environment variable.
HeapChecker: Heap checker is not active, hence checker "trivial" will do nothing!
HeapChecker: To activate set the HEAPCHECK environment variable.
HeapChecker: Heap checker is not active, hence checker "simple" will do nothing!
HeapChecker: To activate set the HEAPCHECK environment variable.
HeapChecker: Heap checker is not active, hence checker "trick" will do nothing!
HeapChecker: To activate set the HEAPCHECK environment variable.
HeapChecker: Heap checker is not active, hence checker "death_simple" will do
nothing!
HeapChecker: To activate set the HEAPCHECK environment variable.
Some liveness flood must be too optimistic
HeapChecker: Heap checker is not active, hence checker "death_loop" will do nothing!
HeapChecker: To activate set the HEAPCHECK environment variable.
Some liveness flood must be too optimistic
./src/tests/heap-checker_unittest.sh: line 59: 23551 Segmentation fault     
$HEAP_CHECKER
make[1]: *** [heap_checker_unittest_sh] Error 139
make[1]: Leaving directory `/home/spot/rpmbuild/BUILD/google-perftools-0.7'
make: *** [check-am] Error 2


Comment 24 Dmitry Butskoy 2006-04-24 14:28:39 UTC
> it compiles and passes 'make check' on my fc3 system
Yep, all is OK now on FC3

But on FC5:
CXXFLAGS="-O2":  coredump
CXXFLAGS="-O1":  "1 of 7 tests failed"

CXX="g++32": "1 of 7 tests failed"

It seems that gcc-4.x is not yet supported well, isn't it?



Comment 25 Dmitry Butskoy 2006-07-17 12:23:12 UTC
0.8

Still coredump on FC5 (still OK on FC3...)

Comment 26 Tom "spot" Callaway 2006-09-09 16:04:17 UTC
Still failing on FC-6. Filed another bug upstream.

Comment 27 Craig Silverstein 2006-12-15 20:00:11 UTC
Sorry it's been so long since I've posted here; I've been hoping to be able to
wait until I have something useful to say.  But I figured I'd give an
end-of-year update in any case.  We've made a lot of progress on getting the
heap-checker and -profiler to work on modern kernels; it basically involved
rewriting that functionality from scratch.  We still have a few more changes
pending, and the folks working on it have been overwhelmed by other projects, so
it hasn't gone as quickly as I'd like. :-(

That said, the tcmalloc_minimal library should work on all FC versions, as far
as I know.  So it would be possible to push that library, and wait until we have
a fix for all the profiling tools before pushing the full package, if you wanted
to try that.  If there are problems with tcmalloc_minimal, please file them as
separate bugs from the normal 'doesn't work on FC5/6' category, since it would
be a separate issue from the ones we know about.

Comment 28 Tom "spot" Callaway 2006-12-15 20:38:48 UTC
Is there a separate set of tests we should be running which will target
tcmalloc_minimal as opposed to the entire perftools suite?

Comment 29 Craig Silverstein 2007-04-18 23:56:33 UTC
Tom, sorry I didn't respond to your query last December. I thought I was set up
to get email every time someone added to this bug report, but I don't, and I
didn't see this until I logged in again just now.

In any case, perftools-0.90 was just released --
http://code.google.com/p/google-perftools.  I've tested it on every Redhat
system from FC2-FC6, and it compiles on all of them and all tests pass.  So I'm
excited for you to take another look and see how things work now.

There's a silly typo-bug that affects x86_64 systems.  I plan to release
perftools-0.91 today to fix that, but if you're testing on 32-bit systems you
should feel free to download the 0.90 version even now.

Comment 30 Tom "spot" Callaway 2007-04-19 00:04:14 UTC
Thanks for the update. :) I think this may be the oldest pending package in the
Fedora queue. I'll wait for 0.91 (all of my immediately available test boxes are
x86_64) to repackage and test.

Comment 31 Craig Silverstein 2007-04-19 00:54:05 UTC
Yikes!  I feel bad about how long things have taken.  But I'm very happy with
the result.

perftools 0.91 is now up on http://code.google.com/p/google-perftools -- you
should be all set to go.

Comment 32 Craig Silverstein 2007-04-19 00:55:11 UTC
btw, please be sure to read the INSTALL file, and perhaps the README as well, if
you're building for x86_64 systems.  There are a few known issues there.

Comment 33 Tom "spot" Callaway 2007-04-24 03:25:05 UTC
Alright, lets get this one off the queue.

Here are new packages, I had to disable smp_mflags (it builds out of order on
SMP), and patch out the use of rpath. But, it does pass all of its tests!

New SRPM:
http://www.auroralinux.org/people/spot/review/google-perftools-0.91-1.fc7.src.rpm
New SPEC:
http://www.auroralinux.org/people/spot/review/google-perftools.spec

Dmitry, if you're no longer interested in reviewing this, I'd understand. Just
lemme know.


Comment 34 Craig Silverstein 2007-04-24 06:22:33 UTC
Great news!

} Here are new packages, I had to disable smp_mflags (it builds out of order on
} SMP)

Is this a bug on our end?  If so, we'd be happy to try to fix it if you can
describe the problem in a bit more detail.

Comment 35 Dmitry Butskoy 2007-04-24 11:12:33 UTC
I'm still here... ;)

All checks are OK under FC5 i386 !


Questions:

It seems that libunwind is not strongly required.
I've successfully built google-perftools under FC5/i386 without it at all. Since
there is no libunwind in FC5 and FC6 distros, and since the trying to build it
under FC5/i386 was failed (due to failed checks), maybe drop this BR, or at
least use this BR for particular arch (64) only?

Patching of both Makefile.am and Makefile.in files looks like not very good way,
AFAIK in such a case we should patch *.am and run some autotools to update *.in 

Are there some more elegant ways for adding of libstacktrace to link stage?
Maybe some unique change is possible (i.e. common LIBS, or something in
Makefile.am and autoconf etc.)?


Comment 36 Tom "spot" Callaway 2007-04-24 14:54:38 UTC
(In reply to comment #34)
> Great news!
> 
> } Here are new packages, I had to disable smp_mflags (it builds out of order on
> } SMP)
> 
> Is this a bug on our end?  If so, we'd be happy to try to fix it if you can
> describe the problem in a bit more detail.

Yeah, this is a bug on your end (a minor one). Try building with make -j3
(doesn't need to be on an SMP system). On my end, the libraries don't finish
building before the binaries try to link, and thus, it fails.


Comment 37 Tom "spot" Callaway 2007-04-24 14:57:44 UTC
(In reply to comment #35)
> I'm still here... ;)
> 
> All checks are OK under FC5 i386 !
> 
> 
> Questions:
> 
> It seems that libunwind is not strongly required.

It is on x86_64, and since it doesn't hurt us to have it everywhere, I made it a
generic BuildRequires.

> I've successfully built google-perftools under FC5/i386 without it at all. Since
> there is no libunwind in FC5 and FC6 distros, and since the trying to build it
> under FC5/i386 was failed (due to failed checks), maybe drop this BR, or at
> least use this BR for particular arch (64) only?

More likely, I'll just remove that BR for FC-5/6 and mark it i386 only for those
dists.

> Patching of both Makefile.am and Makefile.in files looks like not very good way,
> AFAIK in such a case we should patch *.am and run some autotools to update *.in 

Ehh... I'm trying to avoid dragging in autotools. I patched both the .am and the
.in so that it would be clean to anyone who did run autotools off the source.

Comment 38 Craig Silverstein 2007-04-24 22:45:23 UTC
} Yeah, this is a bug on your end (a minor one). Try building with make -j3
} (doesn't need to be on an SMP system). On my end, the libraries don't finish
} building before the binaries try to link, and thus, it fails.

Aha -- I found a problem with missing dependencies for profiler2_unittest and
profiler4_unittest.  Were these the binaries you were seeing trouble with as
well?  Once I fixed those up, everything seemed ok.  I'll have this fixed in the
next release.

Comment 39 Dmitry Butskoy 2007-04-25 11:08:17 UTC
I believe, that period of pregnancy has ended :)

rpmlint: OK
MUST/SHOULD items: OK

APPROVED!

Comment 40 Tom "spot" Callaway 2007-04-25 20:39:30 UTC
New Package CVS Request
=======================
Package Name: google-perftools
Short Description: Very fast malloc & performance analysis tools
Owners: tcallawa
Branches: FC-5 FC-6
InitialCC: 

Comment 41 Tom "spot" Callaway 2007-04-30 03:32:53 UTC
567 days later, this package is finally in Fedora. Thanks to all. :)

Comment 42 Mamoru TASAKA 2008-08-21 13:21:38 UTC
*** Bug 458313 has been marked as a duplicate of this bug. ***


Note You need to log in before you can comment on or make changes to this bug.