i#2006 generalize drcachesim: add the histogram tool

Commit log for first patchset:
---------------
i#2006 generalize drcachesim: add the histogram tool

Adds the histogram tool to track each cacheline usage by the trace.
---------------

[bruening, 2016-09-28 14:46:02]

Please include a test.  That would also show the output for feedback.

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
File clients/drcachesim/tools/histogram.cpp (right):

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
clients/drcachesim/tools/histogram.cpp:44: block_size =
op_line_size.get_value();
Wouldn't "line_size" be a better name?

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
clients/drcachesim/tools/histogram.cpp:45: block_size_bits =
compute_log2(block_size);
And "line_size_bits"

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
clients/drcachesim/tools/histogram.cpp:76: static const size_t N = 10;
A more descriptive name than "N" would help.  Shouldn't this be user-controlled?
 Thus it should be an option?

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
File clients/drcachesim/tools/histogram.h (right):

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
clients/drcachesim/tools/histogram.h:33: /* histogram: represent a memory trace
histogram analysis tool.
nit: s/represent//

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
clients/drcachesim/tools/histogram.h:53: /* Use unsorted  */
Comment is unclear: complete sentence would help.

nit: double space

[zhaoqin, 2016-10-11 18:22:17]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: add the histogram tool

Adds the histogram tool to track each cacheline usage by the trace.

Review-URL: https://codereview.appspot.com/304700043
---------------

[zhaoqin, 2016-10-11 18:30:37]

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
File clients/drcachesim/tools/histogram.cpp (right):

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
clients/drcachesim/tools/histogram.cpp:44: block_size =
op_line_size.get_value();
On 2016/09/28 14:46:02, bruening wrote:
> Wouldn't "line_size" be a better name?

Done,
I was following the block_size used in cache simulator.

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
clients/drcachesim/tools/histogram.cpp:45: block_size_bits =
compute_log2(block_size);
On 2016/09/28 14:46:02, bruening wrote:
> And "line_size_bits"

Done.

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
clients/drcachesim/tools/histogram.cpp:76: static const size_t N = 10;
On 2016/09/28 14:46:02, bruening wrote:
> A more descriptive name than "N" would help.  Shouldn't this be
user-controlled?
>  Thus it should be an option?

Done.

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
File clients/drcachesim/tools/histogram.h (right):

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
clients/drcachesim/tools/histogram.h:33: /* histogram: represent a memory trace
histogram analysis tool.
On 2016/09/28 14:46:02, bruening wrote:
> nit: s/represent//

Done.

https://codereview.appspot.com/304700043/diff/1/clients/drcachesim/tools/hist...
clients/drcachesim/tools/histogram.h:53: /* Use unsorted  */
On 2016/09/28 14:46:02, bruening wrote:
> Comment is unclear: complete sentence would help.
> 
> nit: double space

Done.

[zhaoqin, 2016-10-11 21:29:53]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: add the histogram tool

Adds the histogram tool to track each cacheline usage by the trace.

Review-URL: https://codereview.appspot.com/304700043
---------------

[bruening, 2016-10-12 16:54:04]

LGTM w/ comments

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/analyz...
File clients/drcachesim/analyzer.cpp (right):

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.cpp:132: "Please choose " CPU_CACHE", " TLB", or
"HISTOGRAM".\n");
This is not going to scale, but I guess we agreed we'd make a few more tools and
then take a step back and figure out the model: whether separate libs or what.

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/common...
File clients/drcachesim/common/options.cpp (right):

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/common...
clients/drcachesim/common/options.cpp:200: droption_t<unsigned int>
op_report_tops
The name is weird to me: report_top sounds a lot better (i.e., remove the s)

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
File clients/drcachesim/tools/histogram.cpp (right):

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
clients/drcachesim/tools/histogram.cpp:79: << std::endl;
No need to flush every single time: could just use \n.

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
clients/drcachesim/tools/histogram.cpp:89: << std::dec << it->second <<
std::endl;
Is it clear which number is which?  Maybe a "x" at the end of the count or sthg?

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
File clients/drcachesim/tools/histogram.h (right):

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
clients/drcachesim/tools/histogram.h:53: /* Use unsorted_map (C++11) for faster
lookup */
I think you mean XXX: or sthg: otherwise this is confusing b/c it sounds like
you're describing the current code.

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
clients/drcachesim/tools/histogram.h:59: size_t report_tops;  /* most accessed
lines */
s/tops/top/

https://codereview.appspot.com/304700043/diff/40001/suite/tests/CMakeLists.txt
File suite/tests/CMakeLists.txt (right):

https://codereview.appspot.com/304700043/diff/40001/suite/tests/CMakeLists.tx...
suite/tests/CMakeLists.txt:2417: # Test other analysis tools
Curious, why insert in middle instead of append after offline?

https://codereview.appspot.com/304700043/diff/40001/suite/tests/CMakeLists.tx...
suite/tests/CMakeLists.txt:2418: torunonly_ci(tool.drcachesim.histogram
${ci_shared_app} drcachesim
I was trying to avoid super-long names with "drcacheoff".  Could do the same
here, and highlight that this is not a cache simulation: tool.histogram

[zhaoqin, 2016-10-12 19:02:55]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: add the histogram tool

Adds the histogram tool to track each cacheline usage by the trace.

Review-URL: https://codereview.appspot.com/304700043
---------------

[zhaoqin, 2016-10-12 19:04:30]

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/analyz...
File clients/drcachesim/analyzer.cpp (right):

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.cpp:132: "Please choose " CPU_CACHE", " TLB", or
"HISTOGRAM".\n");
On 2016/10/12 16:54:03, bruening wrote:
> This is not going to scale, but I guess we agreed we'd make a few more tools
and
> then take a step back and figure out the model: whether separate libs or what.

Acknowledged.

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/common...
File clients/drcachesim/common/options.cpp (right):

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/common...
clients/drcachesim/common/options.cpp:200: droption_t<unsigned int>
op_report_tops
On 2016/10/12 16:54:03, bruening wrote:
> The name is weird to me: report_top sounds a lot better (i.e., remove the s)

Done.

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
File clients/drcachesim/tools/histogram.cpp (right):

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
clients/drcachesim/tools/histogram.cpp:79: << std::endl;
On 2016/10/12 16:54:03, bruening wrote:
> No need to flush every single time: could just use \n.

Done.

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
clients/drcachesim/tools/histogram.cpp:89: << std::dec << it->second <<
std::endl;
On 2016/10/12 16:54:03, bruening wrote:
> Is it clear which number is which?  Maybe a "x" at the end of the count or
sthg?

I added 0x for the address, so the output is something like:
Cache Histogram result:
Cache Histogram: icache = 1424 unique cache lines.
Cache Histogram: dcache = 3717 unique cache lines.
Cache Histogram: icache top 10
    0x7fc1fc13ec00: 35735
    0x7fc1fac10a00: 22915
    0x7fc1fac109c0: 21253
    0x7fc1fab16800: 19076
          0x40db40: 17319
          0x406080: 16072
    0x7fc1fc12f880: 15148
    0x7fc1fc12efc0: 14520
          0x406040: 13481
    0x7fc1fc131400: 12524
Cache Histogram: dcache top 10
    0x7ffd63f9e940: 6676
    0x7ffd63f9e600: 6642
    0x7ffd63f9e5c0: 6367
    0x7ffd63f9e900: 6279
    0x7ffd63f9e7c0: 6245
    0x7ffd63f9e8c0: 5949
    0x7ffd63f9e640: 5330
    0x7ffd63f9e580: 5322
    0x7ffd63f9e700: 5268
    0x7ffd63f9e780: 4788

I think it is clearer than adding x at the end of count.

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
File clients/drcachesim/tools/histogram.h (right):

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
clients/drcachesim/tools/histogram.h:53: /* Use unsorted_map (C++11) for faster
lookup */
On 2016/10/12 16:54:03, bruening wrote:
> I think you mean XXX: or sthg: otherwise this is confusing b/c it sounds like
> you're describing the current code.

Done.

https://codereview.appspot.com/304700043/diff/40001/clients/drcachesim/tools/...
clients/drcachesim/tools/histogram.h:59: size_t report_tops;  /* most accessed
lines */
On 2016/10/12 16:54:03, bruening wrote:
> s/tops/top/

Done.

https://codereview.appspot.com/304700043/diff/40001/suite/tests/CMakeLists.txt
File suite/tests/CMakeLists.txt (right):

https://codereview.appspot.com/304700043/diff/40001/suite/tests/CMakeLists.tx...
suite/tests/CMakeLists.txt:2417: # Test other analysis tools
On 2016/10/12 16:54:04, bruening wrote:
> Curious, why insert in middle instead of append after offline?

I was trying to make online tests all together.

https://codereview.appspot.com/304700043/diff/40001/suite/tests/CMakeLists.tx...
suite/tests/CMakeLists.txt:2418: torunonly_ci(tool.drcachesim.histogram
${ci_shared_app} drcachesim
On 2016/10/12 16:54:03, bruening wrote:
> I was trying to avoid super-long names with "drcacheoff".  Could do the same
> here, and highlight that this is not a cache simulation: tool.histogram

Done.

[zhaoqin, 2016-10-12 19:29:14]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/141824d0ce506723622b538a7816f5e...

Final commit log: 
---------------
i#2006 generalize drcachesim: add the histogram tool

Adds the histogram tool to track each cacheline usage by the trace.

Review-URL: https://codereview.appspot.com/304700043
---------------