Re: [PATCH 0/2 RFC] Add time stamp collection to operf and opreport

On 10/02/2015 12:39 PM, Carl E. Love wrote:
> Will, Ramya, OProfile community:
> 
> As mentioned earlier this week, I have been working on a patch to add
> the ability to collect the time stamp for each sample collected by
> operf.  I started work on this with Maynard Johnson.  The first patch in
> the series adds the command line option -T to operf and opreport to
> handle the collection and processing of time stamps.  The list of time
> stamps for each sample is put into the XML file by opreport if the -T
> option is specified and the time stamps were collected by operf.
> 
> Given the number of samples it is very hard to read the data and make
> sense of it.  So, the second patch is provided to process and graph the
> time stamps.  The patch consists of a shell and perl script.  The perl
> script reads up the data from the XML file, process it and generates a
> command and data file for gnu plot.  The shell script takes the name of
> the XML input file, passes it to the perl script to generate the gnu
> plot files.  The shell script then calls gnuplot to generate the graph
> as a post script file.  Finally, the shell script will call xdg-open to
> display the graph.  These scripts are installed as executables along
> with the operf and opreport binaries to make it easy for the user to
> process the data. A couple of different formats for displaying the data
> is provided as a place to start.  If there are other formats that would
> be of use, they can be added in the future.
> 
> I am posting the patches for community review and comment. Please let me
> know what feed back you have on the patches.  Hopefully, we can add this
> functionality to OProfile.  Thank you for your time.
> 
>                       Carl Love
> 


Hi Carl,

Thanks for the patches.  I starting to look at the patches.  I will
try to have a more complete list of comments before the week is out.

In addition to the man pages there should be an example use in the
manual's doc/oprofile.xml.

It would be really nice if the plotting allowed multiple to be plotted
at the same time, for example cycles, instructions retired, and cache
misses. This would make it easier to spot possible cause of a problem.
Another thing to consider plotting is ratios of counts.  There would
be a lot of utility in visualizing fluctuations in cycles per
instructions or cache misses per instruction.

The process_time_stamps.sh script left some files around in the
directory after running.  It should clean up after itself.  Also it
would be nicer if could feed the output of "opreport -T -X" directly
into the scripts rather than having to write them to a file.

Should the doc/opreport.xsd andlibpp/xml_utils.cpp schema_version be bumped
due to the additional timestamp element?

The following additional include is unneeded:

diff --git a/libdb/db_insert.c b/libdb/db_insert.c
index 6bbd71f..6a55d11 100644
--- a/libdb/db_insert.c
+++ b/libdb/db_insert.c
@@ -16,9 +16,16 @@
 #include <errno.h>
 
 #include "odb.h"
+#include "op_libiberty.h"
 
libop/op_config.h doesn't look like the right places for:

+#define EMPTY_TM_ARRAY  ~0x0ULL
+#define VALID_TM_ARRAY_END  0xA5A5A5A55A5A5A5AULL


One of the things I did was do a coverity check with and without the
patches.  There were some minor issues flagged by coverity:


Error: COMPILER_WARNING:
oprofile-1.1.0/libdb/db_manage.c: scope_hint: In function 'odb_write_timestamps'
oprofile-1.1.0/libdb/db_manage.c:432:2: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
#  write(data->fd, &tm_index, sizeof(tm_index));
#  ^
#  430|   	 * followed by the valid time stamp array marker.
#  431|   	 */
#  432|-> 	write(data->fd, &tm_index, sizeof(tm_index));
#  433|   	tm_index = VALID_TM_ARRAY_END;
#  434|   	write(data->fd, &tm_index, sizeof(tm_index));

Error: COMPILER_WARNING:
oprofile-1.1.0/libdb/db_manage.c:434:2: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
#  write(data->fd, &tm_index, sizeof(tm_index));
#  ^
#  432|   	write(data->fd, &tm_index, sizeof(tm_index));
#  433|   	tm_index = VALID_TM_ARRAY_END;
#  434|-> 	write(data->fd, &tm_index, sizeof(tm_index));
#  435|   
#  436|   	return;

Error: STREAM_FORMAT_STATE:
oprofile-1.1.0/libpp/format_output.cpp:963: format_changed: "hex" changes the format state of "out" for category basefield.
#  961|   				found_timestamps = true;
#  962|   				i++;
#  963|-> 				out << "0x" << hex << *it_tm << " ";
#  964|   			}
#  965|   			break;

Error: INVALIDATE_ITERATOR (CWE-119):
oprofile-1.1.0/libpp/format_output.cpp:953: past_the_end: Function "end" creates an iterator.
oprofile-1.1.0/libpp/format_output.cpp:953: tested_end: "it" testing equal to "count_timestamp.end()".
oprofile-1.1.0/libpp/format_output.cpp:969: deref_iterator: Dereferencing iterator "it" though it is already past the end of its container.
#  967|   	}
#  968|   
#  969|-> 	if (it->first != pclass) {
#  970|   		cerr << "ERROR: xml_formatter::output_time stamp_data() Failed "
#  971|   		     << "to find the Time stamp vector for pclass = "

Error: INVALIDATE_ITERATOR (CWE-119):
oprofile-1.1.0/libpp/xml_utils.cpp:404: past_the_end: Function "end" creates an iterator.
oprofile-1.1.0/libpp/xml_utils.cpp:404: tested_end: "it" testing equal to "summary_tm->end()".
oprofile-1.1.0/libpp/xml_utils.cpp:411: deref_iterator: Dereferencing iterator "it" though it is already past the end of its container.
#  409|   	}
#  410|   
#  411|-> 	if (it->first != pclass) {
#  412|   		cerr << "ERROR: xml_utils::output_tm_data failed to find the Time stamp vector for pclass = " << pclass << endl;
#  413|   	}

Error: STREAM_FORMAT_STATE:
oprofile-1.1.0/libpp/xml_utils.cpp:419: format_changed: "hex" changes the format state of "out" for category basefield.
#  417|   	for(; it_tm != tm_vector.end(); it_tm++) {
#  418|   		i++;
#  419|-> 		out << "0x" << hex << *it_tm << " ";
#  420|   		rtn = true;
#  421|   	}


-Will


------------------------------------------------------------------------------
Full-scale, agent-less Infrastructure Monitoring from a single dashboard
Integrate with 40+ ManageEngine ITSM Solutions for complete visibility
Physical-Virtual-Cloud Infrastructure monitoring from one console
Real user monitoring with APM Insights and performance trend reports 
Learn More http://pubads.g.doubleclick.net/gampad/clk?id=247754911&iu=/4140