scons: Add a target for google-perftools profiling

Information
Submitter:	Andreas Hansson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9226:294dbafc3644
---------------------------
scons: Add a target for google-perftools profiling

This patch adds a new target called 'perf' that facilitates profiling
using google perftools rather than gprof. The perftools CPU profiler
offers plenty useful information in addition to gprof, and the latter
is kept mostly to offer profiling also on non-Linux hosts.

Testing Done

Compiled and ran a few experiments using the perf binary

Issue Summary

Description	From	Last Updated	Status
this is pretty minor, but the formatting on this line is pretty awkward... why not wrap on a comma in ...	Steve Reinhardt	Sept. 14, 2012, 1:10 a.m.	Resolved

Ship It!

Change Summary:

Added some profiling output to exemplify the functionality.

Description:

		Changeset 9213:2c3700a56b60

		scons: Add a target for google-perftools profiling

		This patch adds a new target called 'perf' that facilitates profiling
		using google perftools rather than gprof. The perftools CPU profiler
		offers plenty useful information in addition to gprof, and the latter
		is kept mostly to offer profiling also on non-Linux hosts.
	+
	+	A small update, after running twolf on ARM with the CPU profiler the
	+	output starts with:
	+
	+	Total: 96679 samples
	+	2437 2.5% 2.5% 2561 2.6% TimeBuffer::access (inline)
	+	2266 2.3% 4.9% 2501 2.6% TimeBuffer::advance
	+	2263 2.3% 7.2% 2594 2.7% RefCounted::decref (inline)
	+	2252 2.3% 9.5% 2252 2.3% Stats::StatStor::inc (inline)
	+	2099 2.2% 11.7% 4711 4.9% RefCountingPtr::del (inline)
	+	2087 2.2% 13.9% 2087 2.2% SLL_Pop
	+	1858 1.9% 15.8% 2327 2.4% RefCountingPtr::copy (inline)
	+	1468 1.5% 17.3% 1468 1.5% tc_new
	+	1438 1.5% 18.8% 1438 1.5% tc_delete
	+
	+	For this specific benchmark, the O3 CPU accounts for roughly 85% before
	+	the first memory system function shows up. No free lunch...

This looks fine except for the minor formatting thing... just clean that up a bit and there's no need to resubmit

src/SConscript (Diff revision 1)

this is pretty minor, but the formatting on this line is pretty awkward... why not wrap on a comma in the outer list (which I think could keep this to two lines) rather than bunching everything at the end across four?

or you could do it in two steps, assign the flag list to a variable then do:
dict(zip(target_types, linker_flags)

Show all issues

Andreas Hansson Sept. 14, 2012, 1:17 a.m. (Sept. 14, 2012, 1:17 a.m.)

It wasn't the pretties way for sure. I added a perf_ldflags variable on the line before and thus got it all in two lines now.

Nathan Binkert Sept. 14, 2012, 1:23 a.m. (Sept. 14, 2012, 1:23 a.m.)

I'm bikeshedding here, but I personally would much rather see the dict written out.  I'm not convinced that you've saved more than a dozen characters of typing really, and it certainly is more prone to errors.

Andreas Hansson Sept. 14, 2012, 1:31 a.m. (Sept. 14, 2012, 1:31 a.m.)

Can also go with:

ldflags = dict(zip(target_types, [[], [], [], ['-pg'], 
                                  ['-Wl,--no-as-needed', '-lprofiler', 
                                   '-Wl,--as-needed']]))

Nathan Binkert Sept. 14, 2012, 1:35 a.m. (Sept. 14, 2012, 1:35 a.m.)

No, I meant:
ldflags = { 'debug' : [], 'opt' : [], 'fast' : [], 'prof' : ...

I think the dict(zip()) thing is error prone.

Andreas Hansson Sept. 14, 2012, 1:45 a.m. (Sept. 14, 2012, 1:45 a.m.)

Don't you think that retyping the target_types in all these locations is equally bad if not worse in terms of being error prone?

Steve Reinhardt Sept. 14, 2012, 1:48 a.m. (Sept. 14, 2012, 1:48 a.m.)

I didn't mean to open up this can of worms, but I agree with Nate, the explicit dict, while more verbose, is a lot clearer and more maintainable.

The same goes for obj2target further down.

Steve Reinhardt Sept. 14, 2012, 1:50 a.m. (Sept. 14, 2012, 1:50 a.m.)

The key thing is that if you extend, reorder, or otherwise change target_types, the zip method can potentially fail silently (e.g., by mapping the wrong flags to the wrong target in a way that would be hard for someone other than you to track down), while the dict method will either continue to work correctly or give you an error (e.g., undefined key).

Andreas Hansson Sept. 14, 2012, 2:01 a.m. (Sept. 14, 2012, 2:01 a.m.)

Reshuffled the previous patch as well to use the explicit dict instead of python magic. Hopefully the can of worms is now officially closed :)

Description:

~		Changeset 9213:2c3700a56b60
	~	Changeset 9226:80a0423ec9e5

		scons: Add a target for google-perftools profiling

		This patch adds a new target called 'perf' that facilitates profiling
		using google perftools rather than gprof. The perftools CPU profiler
		offers plenty useful information in addition to gprof, and the latter
		is kept mostly to offer profiling also on non-Linux hosts.
-
-		A small update, after running twolf on ARM with the CPU profiler the
-		output starts with:
-
-		Total: 96679 samples
-		2437 2.5% 2.5% 2561 2.6% TimeBuffer::access (inline)
-		2266 2.3% 4.9% 2501 2.6% TimeBuffer::advance
-		2263 2.3% 7.2% 2594 2.7% RefCounted::decref (inline)
-		2252 2.3% 9.5% 2252 2.3% Stats::StatStor::inc (inline)
-		2099 2.2% 11.7% 4711 4.9% RefCountingPtr::del (inline)
-		2087 2.2% 13.9% 2087 2.2% SLL_Pop
-		1858 1.9% 15.8% 2327 2.4% RefCountingPtr::copy (inline)
-		1468 1.5% 17.3% 1468 1.5% tc_new
-		1438 1.5% 18.8% 1438 1.5% tc_delete
-
-		For this specific benchmark, the O3 CPU accounts for roughly 85% before
-		the first memory system function shows up. No free lunch...

Diff:

Revision 2 (+32 -18)

Show changes

src/SConscript

Ship It!

Description:

~		Changeset 9226:80a0423ec9e5
	~	Changeset 9226:294dbafc3644

		scons: Add a target for google-perftools profiling

		This patch adds a new target called 'perf' that facilitates profiling
		using google perftools rather than gprof. The perftools CPU profiler
		offers plenty useful information in addition to gprof, and the latter
		is kept mostly to offer profiling also on non-Linux hosts.

Diff:

Revision 3 (+28 -15)

Show changes

src/SConscript

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Change Summary:

Description:

Changeset 9213:2c3700a56b60

Description:

Changeset 9213:2c3700a56b60

Changeset 9226:80a0423ec9e5

Diff:

Description:

Changeset 9226:80a0423ec9e5

Changeset 9226:294dbafc3644

Diff:

Status: Closed (submitted)