i#3129 raw2trace perf: summarize instr_t during raw2trace conversion #3130

mtrofin · 2018-08-20T17:14:37Z

Summarize instr_t to decouple trace conversion from DR internals (e.g. instru_t APIs)

As added benefit, the previous APIs were not inlined (at least by clang), being in a different module. That was resulting in noticeable (under perf profiling) function call overhead. This change alleviates that.

Issue: #3129

Summarize instr_t to decouple trace conversion from DR internals (e.g. instru_t APIs) Issue: DynamoRIO#3129

derekbruening · 2018-08-20T18:11:42Z

As added benefit, the previous APIs were not inlined (at least by clang), being in a different module. That was resulting in noticeable (under perf profiling) function call overhead. This change alleviates that.

The different module shouldn't matter if DynamoRIO_FAST_IR is turned on, is it turns the calls into macros. Was that tried? It applies to the common IR accessors only so maybe the issue here is that uncommon calls like instr_is_cti are being used which are not currently part of DynamoRIO_FAST_IR. Not sure it should be extended just for this use case.

mtrofin · 2018-08-20T18:34:59Z

The main justification is actually the decoupling - when we templatize the parts of raw2trace_t we need, we'd end up with needing to pull instru_t into raw2trace.h.

derekbruening · 2018-08-20T23:56:54Z

clients/drcachesim/tracer/raw2trace.h

@@ -71,6 +71,128 @@ struct module_t {
    bool is_external; // If true, the data is embedded in drmodtrack custom fields.
 };

+/**
+ * instr_summary is a compact encapsulation of the information needed by trace conversion


derekbruening · 2018-08-21T00:07:44Z

clients/drcachesim/tracer/raw2trace.h

+    app_pc next_pc_ = 0;
+
+    std::vector<opnd_t> srcs_and_dests_;
+    uint8_t srcs_ = 0;


num_mem_srcs_ or count_mem_srcs_ is more descriptive: o/w this seems like the actual list of them. Ditto for dests.

derekbruening · 2018-08-21T00:13:34Z

clients/drcachesim/tracer/raw2trace.h

+    instr_summary_t &
+    operator=(instr_summary_t &&) = delete;
+
+    uint16_t type_ = 0;


Note that an _ suffix is not used anywhere else to mark class members: in fact for drcachesim _ is used in kind of the opposite way to distinguish param vars to constructors. I don't feel strongly, ok to leave.

derekbruening · 2018-08-21T00:14:38Z

clients/drcachesim/tracer/raw2trace.h

+        return srcs_and_dests_[srcs_ + pos];
+    }
+    size_t
+    srcs() const


num_mem_srcs() or count_memory_srcs() or sthg is more descriptive: see below too. Ditto for dsts.

derekbruening · 2018-08-21T00:24:51Z

clients/drcachesim/tracer/raw2trace.h

+    const opnd_t &
+    dest_at(size_t pos) const
+    {
+        return srcs_and_dests_[srcs_ + pos];


Combining srcs and dsts just seems to add complexity: is there something positive coming out of it? Maybe best to separate.

One less large object (another std::vector is larger than the 2 bytes). This helps in an implementation where we don't allocated individual instr_summary_t on the heap, but rather bulk-allocate entire pages of them.

In fact, we only need the nr of sources.

derekbruening · 2018-08-21T00:32:10Z

clients/drcachesim/tracer/raw2trace.h

+    byte length_ = 0;
+    app_pc next_pc_ = 0;
+
+    std::vector<opnd_t> srcs_and_dests_;


Looking at the implementation and use, these are actually only the memory operands, while the name here sounds like all operands. Better to use memory_srcs_ and memory_dsts_ or sthg to make this clear.

derekbruening · 2018-08-21T00:32:38Z

clients/drcachesim/tracer/raw2trace.h

+    }
+
+    const opnd_t &
+    src_at(size_t pos) const


memory_src_at() or sthg -- see also other comments. Ditto for dest.

derekbruening · 2018-08-21T00:33:34Z

clients/drcachesim/tracer/raw2trace.cpp

+            (instr->reads_memory() || instr->writes_memory())) {
+            for (uint j = 0; j < instr->srcs(); j++) {
+                std::string error =
+                    append_memref(&buf, tidx, instr, instr->src_at(j), false);


You're missing the check for being a memory ref.
UPDATE: see below: srcs and src_at would be much clearer with names like num_mem_srcs and mem_src_at or sthg.

done, renamed

derekbruening · 2018-08-21T00:38:09Z

clients/drcachesim/tracer/raw2trace.cpp

+        desc->srcs_and_dests_.resize(desc->srcs_ + desc->dests_);
+        int pos = 0;
+        TRAVERSE_SRCS(desc->srcs_and_dests_[pos++] = op)
+        TRAVERSE_DSTS(desc->srcs_and_dests_[pos++] = op)


Simpler code that just does a loop with push_back and no macros will be more readable and maintainable, and I wouldn't think any slight overhead here would matter: unless there's profiling evidence I'd rather not have macros and less readable code on a non-critical-path like this.

true, and we can always rework this if it pops on any perf radar. reworked.

derekbruening · 2018-08-21T00:42:06Z

clients/drcachesim/tracer/raw2trace.h

+    FIELD(IsPrefetch, 2)
+    FIELD(IsFlush, 3)
+    FIELD(IsCti, 4)
+#undef FIELD


This is another one where the style used in all existing DR code of hex constants

... kIsPrefetchMask = 0x0002; kIsFlushMask = 0x0004; kIsCtiMask = 0x0008;

and setting it via

if (instr_is_prefetch(instr)) desc->packed_ |= kIsPrefetchMask;

seems simpler and more readable to me, avoids extra macros, doesn't rely on "bool" being 1 (such assumptions lead to bugs in C code), and matches the existing style. WDYT?

derekbruening · 2018-08-21T15:41:28Z

clients/drcachesim/tracer/raw2trace.cpp

@@ -878,8 +878,7 @@ raw2trace_t::get_instr_summary(uint64 modidx, uint64 modoffs, INOUT app_pc *pc,
        hashtable_add(&decode_cache, decode_pc, desc);
        ret = desc;
    } else {
-        // TODO(mtrofin): log some rendering of the instruction summary that will be
-        // returned
+        // Log some rendering of the instruction summary that will be returned. i#3129


I don't understand the comment: there is no log here. This was a "TODO" which as my prior comment said is "XXX" in the DR style (see the wiki) and I assume it is still a comment about future work, right? Repeating what that should look like in the DR style: XXX i#3129: Log ... .

ooh! I see... I kept looking in the wiki for "TODO" (assuming xxx referred to "substitute text here"). Didn't find anything about "todo", so assumed you were describing the style. Anyway. I think I understand now.

derekbruening · 2018-08-21T15:43:38Z

clients/drcachesim/tracer/raw2trace.h

-     * orig_pc and verbosity)
+     * Populates a pre-allocated instr_summary_t description, from the instruction found
+     * at pc. Updates pc to the next instruction. Optionally logs translation details
+     * (using orig_pc and verbosity)


style: missing trailing .

derekbruening · 2018-08-21T15:44:29Z

clients/drcachesim/tracer/raw2trace.h

-    std::vector<opnd_t> srcs_and_dests_;
-    uint8_t srcs_ = 0;
-    uint8_t dests_ = 0;
+    std::vector<opnd_t> mem_srcs_and_dests_;


Add a comment explaining why this squishes the srcs and dests together. (Otherwise someone is going to split them up, seeing no reason to combine them.)

derekbruening · 2018-08-21T15:53:09Z

clients/drcachesim/tracer/raw2trace.cpp

@@ -72,7 +72,7 @@

 #define DO_VERBOSE(level, x)              \
    do {                                  \
-        if (this->verbosity >= (level)) { \
+        if (verbosity >= (level)) {       \


Hmm, this assumed var verbosity seems a little odd and fragile when it doesn't exist anywhere except in one function. Maybe verbosity should be a parameter to the macro? (And then in ()'s when refed of course.)

derekbruening · 2018-08-21T17:32:33Z

clients/drcachesim/tracer/raw2trace.cpp

@@ -878,7 +878,8 @@ raw2trace_t::get_instr_summary(uint64 modidx, uint64 modoffs, INOUT app_pc *pc,
        hashtable_add(&decode_cache, decode_pc, desc);
        ret = desc;
    } else {
-        // Log some rendering of the instruction summary that will be returned. i#3129
+        /* XXX i#3129 Log some rendering of the instruction summary that will be returned.


nit: missing :

derekbruening · 2018-08-21T17:35:17Z

clients/drcachesim/tracer/raw2trace.cpp

+                   instr_set_translation(instr, orig_pc);
+                   dr_print_instr(dcontext, STDOUT, instr, "");
+               },
+               verbosity);


Seems easier to have verbosity ordered before the statement?

The macro is used exactly in one place, so chose to delete the macro and write the explicit check - imho, easier to understand, and one less macro.

derekbruening · 2018-08-21T18:43:18Z

The commit title is not the right style -- please see https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews#commit-messages for future commits.

derekbruening · 2018-08-21T20:54:29Z

tool.drcachesim.missfile-config-file failed on Appveyor. c3d5b47 was just committed which changes the missfile behavior -- yet it doesn't seem like it could cause this failure as it doesn't change the simulator output. Strange that it never failed before though. cc @kharbutli. Going to commit this and filed #3136 to track the test failure.

i#3129 Summarize instr_t during raw2trace conversion

2ae2c85

Summarize instr_t to decouple trace conversion from DR internals (e.g. instru_t APIs) Issue: DynamoRIO#3129

mtrofin added 5 commits August 20, 2018 11:37

Formatting

2ac7472

Use TESTANY

14e010a

TEMP: increase verbosity on appveyor

cd06b8d

Fixed conversion warnings on windows

93f6063

Undo runsuite_wrapper changes

9cae014

derekbruening reviewed Aug 21, 2018

View reviewed changes

Incorporated feedback

14e5371

derekbruening reviewed Aug 21, 2018

View reviewed changes

Feedback

ea332a6

derekbruening reviewed Aug 21, 2018

View reviewed changes

Feedback

582dee0

derekbruening approved these changes Aug 21, 2018

View reviewed changes

mtrofin changed the title ~~i#3129 Summarize instr_t during raw2trace conversion~~ i#3129 raw2trace perf: summarize instr_t during raw2trace conversion Aug 21, 2018

Merge branch 'master' into master

2a2048b

derekbruening merged commit 9d849e2 into DynamoRIO:master Aug 21, 2018

i#3129 raw2trace perf: summarize instr_t during raw2trace conversion #3130

i#3129 raw2trace perf: summarize instr_t during raw2trace conversion #3130

Conversation

mtrofin commented Aug 20, 2018

derekbruening commented Aug 20, 2018

mtrofin commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrofin Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbruening commented Aug 21, 2018

derekbruening commented Aug 21, 2018

mtrofin Aug 21, 2018 •

edited

Loading