i#2006 generalize drcachesim: add threading pool support for writing out traces by zhaoqin · Pull Request #2319 · DynamoRIO/dynamorio

zhaoqin · 2017-03-30T20:59:34Z

Adds two options -num_threads and -queue_size to the tracer for asynchronous
trace write by the sideline threads.
This CL only adds the two options, and the real implementation is to be added.

… write Adds two options -num_threads and -queue_size to tracer for asynchronous trace write by sideline threading pool. This CL only adds the two options, real implementation is to be added.

derekbruening

spelling in title: "drcachesime"

Small incremental reviews are good in general but the repository has to be in a valid state at any time, so unimplemented options need to be labeled as unimplemented to avoid confusion. But maybe it's better to only add an option when there's something implemented that it controls.

derekbruening · 2017-03-30T22:11:40Z

+ "0 means synchronized write without any sideline threads.");
+
+droption_t<unsigned int> op_queue_size
+(DROPTION_SCOPE_ALL, "queue_size", 4000, "The queue size for ",


derekbruening · 2017-03-30T22:12:39Z

+droption_t<unsigned int> op_queue_size
+(DROPTION_SCOPE_ALL, "queue_size", 4000, "The queue size for ",
+ "For the offline analysis mode (when -offline is requested), specifies the size "
+ "of the queue for batching trace data to be written out by sideline threads.");


What are the units here? If bytes this is way too small. If buffers it seems large for a default: 128MB for 32K buffers.

derekbruening · 2017-03-30T22:13:24Z

+droption_t<bytesize_t> op_num_threads
+(DROPTION_SCOPE_ALL, "num_threads", 0, "Number of threads for writing traces",
+ "For the offline analysis mode (when -offline is requested), specifies the number "
+ "of sideline threads are used to write traces file out.  "


grammar: s/are/to be/
grammar: s/traces file/trace files/

derekbruening · 2017-03-30T22:14:01Z

 "for later offline analysis.  No simulator is executed.");

+droption_t<bytesize_t> op_num_threads
+(DROPTION_SCOPE_ALL, "num_threads", 0, "Number of threads for writing traces",


Shouldn't this be SCOPE_CLIENT?

derekbruening · 2017-03-30T22:14:12Z

+ "0 means synchronized write without any sideline threads.");
+
+droption_t<unsigned int> op_queue_size
+(DROPTION_SCOPE_ALL, "queue_size", 4000, "The queue size for ",


Just SCOPE_CLIENT, right?

derekbruening · 2017-03-30T22:19:56Z

        torunonly_ci(tool.drcacheoff.${testname} ${exetgt} drcachesim
          "offline-${testname}.c" # for templatex basename
-          "-offline" "" "")
+          "-offline -num_threads 0" "" "")


Isn't this the default?

was trying to test if the option can be specified.
removed.

Adds two options -num_threads and -queue_size to the tracer for asynchronous trace write by the sideline threads. This CL only adds the two options, and the real implementation is to be added.

zhaoqin · 2017-03-31T02:04:07Z

PTAL

Adds two options -num_threads and -queue_size to the tracer for asynchronous trace write by the sideline threads. This CL only adds the two options, and the real implementation is to be added.

This CL uses client sideline threads to overlap the profiling and data output. We use a producer-consumer queue to pass traces from profiling thread to sideline threads. Tow options -num_threads and -queue_size are added to specify the threading pool parameters. A simple sideline test is also added. TODO: enable sideline thread for burst and static tests.

derekbruening

Part-way through.

Offline we talked a little about whether this needs to be limited to offline -- seems like the pool can be positioned at a point shared by both.

derekbruening · 2017-04-14T20:36:50Z

        per_thread_t *data = (per_thread_t *) drmgr_get_tls_field(drcontext, tls_idx);
        ssize_t size = towrite_end - towrite_start;
-        if (file_ops_func.handoff_buf != NULL) {
+        if (op_num_threads.get_value()) {


style violation: non-bool as bool

derekbruening · 2017-04-14T20:40:27Z


+    /* setup sideline thread and queue */
+    if (op_offline.get_value() && op_num_threads.get_value() > 0) {
+        /* XXX support thread pool with more than one threads */


nit: "XXX:"
nit: "one thread"

add multi-thread support

derekbruening · 2017-04-14T20:42:13Z

+        /* XXX support thread pool with more than one threads */
+        if (op_num_threads.get_value() > 1) {
+            NOTIFY(0, "Usage error: only one sideline thread is supported");
+            DR_ASSERT(false);


Should be dr_abort() if fatal -- that's what the other usage errors do

derekbruening · 2017-04-14T20:42:51Z

+    uint i;
+    queue_size = op_queue_size.get_value() / max_buf_size;
+    // We round up queue_size to the nearest power of 2.
+    for (i = 1; i <= queue_size; i <<= 1) /*do nothing*/;


style violation

derekbruening · 2017-04-14T20:48:20Z

-        set(tool.drcacheoff.${testname}_basedir
+      macro (torunonly_drcacheoff testname exetgt sideline)
+        if (${sideline})
+          set(tname "${testname}-sideline")


nit: these are all using . and _ rather than - as a separator: "tool.drcacheoff.burst_static"

removed since the default is the sideline.
Will add test without sideline

derekbruening · 2017-04-14T20:49:41Z

-        set(tool.drcacheoff.${testname}_basedir
+      macro (torunonly_drcacheoff testname exetgt sideline)
+        if (${sideline})
+          set(tname "${testname}-sideline")


Rather than exploding the option space, can we just make the thread pool the default? This will make testing much simpler.

derekbruening · 2017-04-14T20:50:24Z

 "For the offline analysis mode (when -offline is requested), specifies the number "
 "of sideline threads to be used to write trace files out.  "
- "0 means synchronized write without any sideline threads.");
+ "Only 0 and 1 is currently supported.  "


grammar: are

derekbruening · 2017-04-14T21:02:49Z

- "(NYI) The maximum size of the queue for traces before they are written out",
- "For the offline analysis mode (when -offline is requested), specifies the maximum "
- "size (number of bytes) of the queue for traces before they are written out "
+ "The approximate maximum size of the queue for traces before they are written out",


As discussed offline: the description can be clearer. As written it sounds like a minimum size, where nothing is written at all until it's reached. Really what you mean is the size at which we'll block the app threads and wait for the queue to shrink.

derekbruening · 2017-04-15T04:14:58Z

+static volatile uint queue_tail = 0;
+static void *sideline_exit_event;
+struct buf_entry_t {
+    file_t  file;  // file descriptor, 0 means the end of profiling.


nit: INVALID_FILE, not 0

derekbruening

It seems better to support multiple threads now, which is just a couple of lines more of code, and ensures the design won't have to change. It looks like this CL's design will have to change, which we would avoid by having multiple threads to start with.

derekbruening · 2017-04-15T04:24:39Z


+/* sideline */
+static drvector_t buf_queue;
+static uint queue_size = 0;


suggest: comment explaining what this is: probably current total size of all buffers, rather than the length of the vector?

derekbruening · 2017-04-15T04:25:20Z

+static uint queue_size = 0;
+static uint queue_size_mask = 0;
+static volatile uint queue_front = 0;
+static volatile uint queue_tail = 0;


I'm guessing this is a circular buffer and these are array indices? Suggest: comments.

derekbruening · 2017-04-15T22:23:39Z

+/* sideline */
+static drvector_t buf_queue;
+static uint queue_size = 0;
+static uint queue_size_mask = 0;


Hmm, no, if this is based on queue_size, then queue_size must be the length of the vector? Why do we need a separate var for that: it's already in buf_queue, right?

derekbruening · 2017-04-15T22:23:55Z

+static uint queue_size_mask = 0;
+static volatile uint queue_front = 0;
+static volatile uint queue_tail = 0;
+static void *sideline_exit_event;


Don't we need a separate one per thread?

derekbruening · 2017-04-15T22:24:51Z

+        }
+        drvector_lock(&buf_queue);
+        if (((queue_front + 1) & queue_size_mask) != queue_tail) {
+            drvector_set_entry(&buf_queue, queue_front, entry);


So the invariant is that the front is empty? Maybe a comment on that up above? How about an assert that the current front file is INVALID_FILE?

Was this done?

add comment on the queue_front variable.

derekbruening · 2017-04-15T22:36:36Z

-        if (file_ops_func.handoff_buf != NULL) {
+        if (op_num_threads.get_value()) {
+            trace_buf_enqueue(data->file, towrite_start, size);
+        } else if (file_ops_func.handoff_buf != NULL) {


So you've put a priority here: the handoff is going to be ignored if there's a thread pool. That should be reflected in its docs.

Maybe we should just remove the handoff feature completely, since it's not needed anymore: it can be duplicated by using a thread pool and having file_ops_func.write do whatever you want.

I also think the thread pool should be the default.

removed handoff

derekbruening · 2017-04-15T22:38:38Z

+ "For the offline analysis mode (when -offline is requested), specifies the number "
+ "of sideline threads to be used to write trace files out.  "
+ "Only 0 and 1 is currently supported.  "
+ "0 means synchronized write without any sideline threads.  ");


IMHO the pool should be on by default, for code simplicity.

I'm not sure about only supporting 1 thread: we need to know that the design here supports many threads, and seeing the stop-profiling scheme below I'm not sure that it does. Adding >1 thread seems like just a few lines of code: array of events, for loop when touching those events. It seems better to include multi-thread support now, to ensure the design works (again see below where I'm not sure it does).

A threading pool with more than one thread has a complex problem, i.e, sideline thread vs open files.
The simplest way is one open file per sideline thread, which requires us to change raw2trace.
If we want to keep current one file per app-thread, writing to same file with multiple threads is complex.
I will go with the one file per sideline thread.

I don't remember seeing any mention of this in the CL? Did I miss it? E.g., the scheme for putting an entry in the queue to terminate the sideline thread function isn't marked as temporary, so the reader can't tell why only one thread is supported and why we can't just add an array and for loops to easily increase the pool to 100 threads.

set it default.

derekbruening · 2017-04-15T22:39:41Z

+    if (op_offline.get_value()) {
+        if (op_num_threads.get_value() > 0) {
+            trace_buf_enqueue((file_t)0, NULL, 0);  // end of profiles
+            dr_event_wait(sideline_exit_event);


Needs to be a for loop, one event per thread, right?

derekbruening · 2017-04-15T22:41:33Z

+    queue_size = i;
+    if (queue_size < 2) {
+        NOTIFY(0, "Usage error: queue size too small\n");
+        DR_ASSERT(false);


There are other comments like this that I did not see any reply to.

derekbruening · 2017-04-15T22:42:09Z

+        get_queue_size();
+        drvector_init(&buf_queue, queue_size, false, NULL);
+        dr_create_client_thread(sideline_run, NULL);
+        sideline_exit_event = dr_event_create();


for loop around these two lines

Added threading pool for asynchronous trace dump. - used a circular buffer for producer consumer queue, - added fork handling, - updated trace format to write trace per sideline thread instead of app thread, - removed handoff callbacks.

zhaoqin · 2017-04-25T01:04:56Z

Just updated the threading pool implementation.
PTAL.

zhaoqin · 2017-04-25T02:46:45Z

I put comment for queue_front/queue_tail (replaced with queue_put/queue_get), which I feel clearer.

…

On Mon, Apr 24, 2017 at 9:30 PM, Derek Bruening ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In clients/drcachesim/tracer/tracer.cpp <#2319 (comment)>: > +trace_buf_enqueue(file_t file, void *data, ssize_t size) +{ + bool done = false; + buf_entry_t *entry = (buf_entry_t *)dr_global_alloc(sizeof(*entry)); + entry->file = file; + entry->data = data; + entry->size = size; + do { + /* quick racy check without locking */ + while (((queue_front + 1) & queue_size_mask) == queue_tail) { + /* the queue is full, waiting */; + dr_thread_yield(); + } + drvector_lock(&buf_queue); + if (((queue_front + 1) & queue_size_mask) != queue_tail) { + drvector_set_entry(&buf_queue, queue_front, entry); Was this done? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2319 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABkcDPeoJc9n0--B16LYEo6jU2371lx6ks5rzUy6gaJpZM4Mu6lp> .

zhaoqin · 2017-04-25T02:47:15Z

I kind of rewriting a lot, including removing entry->file.

…

On Mon, Apr 24, 2017 at 9:30 PM, Derek Bruening ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In clients/drcachesim/tracer/tracer.cpp <#2319 (comment)>: > +{ + buf_entry_t *entry; + bool profiling = true; + do { + /* quick racy check without locking */ + while (queue_tail == queue_front) { + /* the queue is empty, waiting */ + dr_thread_yield(); + } + entry = trace_buf_dequeue(); + if (entry == NULL) + continue; + if (entry->data != NULL) { + file_ops_func.write_file(entry->file, entry->data, entry->size); + dr_raw_mem_free(entry->data, max_buf_size); + } else if (entry->file != 0) { This one? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2319 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABkcDEldw-0AzHlp_15RdV0_fdw_1Qcoks5rzUzBgaJpZM4Mu6lp> .

derekbruening

Looks pretty good though there seem to be a few issues: memory leak, ignoring return values, etc.. Most comments are suggestions or style nits.

Don't really want to spend more time it so approving with requests for changes.

derekbruening · 2017-04-25T03:02:59Z

    virtual int append_thread_exit(byte *buf_ptr, thread_id_t tid) = 0;
    virtual int append_iflush(byte *buf_ptr, addr_t start, size_t size) = 0;
-    virtual int append_thread_header(byte *buf_ptr, thread_id_t tid) = 0;
+    // This is a per-thread-file-writeout header.


"per-thread-file-writeout" means every time we write to a per-thread file -- so how would that be different from the per-buffer-writeout header? Do you mean just "per-thread-file"?

done
I was trying to say the write-out/sideline thread instead of app thread.

derekbruening · 2017-04-25T03:05:20Z

 "for later offline analysis.  No simulator is executed.");

+droption_t<unsigned int> op_num_threads
+(DROPTION_SCOPE_CLIENT, "num_threads", 1, "Number of sideline threads for writing traces",


Why 1? It seems too small. How was it chosen?

We set it 1 so the online simulation would work here.
could this been dynamically set here?
Ideally, it would be something like 1/4 of number of cpus or something. Any cross-platform way to get number of CPUs?

derekbruening · 2017-04-25T18:42:10Z


 int
-offline_instru_t::append_thread_header(byte *buf_ptr, thread_id_t tid)
+offline_instru_t::append_thread_file_header(byte *buf_ptr)


Kind of confusing to have "thread" in the name: makes me wonder why there's a pid in the header but no tid? Maybe just "append_file_header".

derekbruening · 2017-04-25T18:45:05Z

      torunonly_ci(tool.reuse ${ci_shared_app} drcachesim
        "reuse_distance.c" # for templatex basename
-        "-ipc_name drtestpipe6 -simulator_type reuse_distance -reuse_distance_threshold 256" "" "")
+        "-ipc_name drtestpipe6 -num_threads 0 -simulator_type reuse_distance -reuse_distance_threshold 256" "" "")


? Why no thread pool?

We had the thread pool test for drcachesim online simulation
Here I want to test the online simulation without threading pool, unless we do not need to support that any more.

OK but it is confusing b/c this looks like there's some relationship to the reuse-distance tool: like the reuse-distance tool doesn't support the thread pool or sthg. I would suggest either a comment explaining, or adding a new test run that looks just an existing run except it has -num_threads 0.

done, add a separate test instead.

derekbruening · 2017-04-25T18:45:39Z

      prefix_cmd_if_necessary(drcachesim_path ON ${drcachesim_path})

-      macro (torunonly_drcacheoff testname exetgt)
+      macro (torunonly_drcacheoff testestname exetgt)


? "testestname"? The original is better: is this is a mistake?

done
my stupid reg replace mistake.

derekbruening · 2017-04-25T20:26:10Z

+        dr_global_alloc(op_num_threads.get_value() * sizeof(sideline_exit_events[0]));
+    for (i = 0; i < op_num_threads.get_value(); i++) {
+        sideline_exit_events[i] = dr_event_create();
+        sideline_args[i].id   = i;


nit: extra spaces

derekbruening · 2017-04-25T20:26:33Z

+        sideline_args[i].id   = i;
+        if (op_offline.get_value())
+            sideline_args[i].file = create_thread_file((ptr_int_t)i);
+        dr_create_client_thread(sideline_run, &sideline_args[i]);


Check the return value

derekbruening · 2017-04-25T20:27:38Z

+        }
+        if (op_num_threads.get_value() > 1) {
+            NOTIFY(0, "Usage error: only one sideline thread is supported "
+                   "for online simulation\n");


Why? Please explain in a comment: is this temporary or permanent?
Is this documented in options.cpp?

done.
only one named pipe, so one thread makes sense, right?

Will something not work correctly if there's more than one thread in the pool? Or it will work, you just think there's no perf improvement?

derekbruening · 2017-04-25T20:30:28Z

+            entry->data = data;
+            entry->size = size;
+            entry->tid = tid;
+            drvector_set_entry(&circular_buf_queue, queue_put, entry);


Isn't this redundant?

derekbruening · 2017-04-25T20:31:24Z

+    queue_get = 0;
+}
+
+void event_fork_init(void *drcontext)


style violation: \n

Also, should be static

Added threading pool for asynchronous trace dump. - used a circular buffer for producer consumer queue, - added fork handling, - updated trace format to write trace per sideline thread instead of app thread, - removed handoff callbacks.

zhaoqin

updated based on the review.

zhaoqin · 2017-04-26T00:20:22Z

 "for later offline analysis.  No simulator is executed.");

+droption_t<unsigned int> op_num_threads
+(DROPTION_SCOPE_CLIENT, "num_threads", 1, "Number of sideline threads for writing traces",


We set it 1 so the online simulation would work here.
could this been dynamically set here?
Ideally, it would be something like 1/4 of number of cpus or something. Any cross-platform way to get number of CPUs?

zhaoqin · 2017-04-26T00:24:19Z

    virtual int append_thread_exit(byte *buf_ptr, thread_id_t tid) = 0;
    virtual int append_iflush(byte *buf_ptr, addr_t start, size_t size) = 0;
-    virtual int append_thread_header(byte *buf_ptr, thread_id_t tid) = 0;
+    // This is a per-thread-file-writeout header.


done
I was trying to say the write-out/sideline thread instead of app thread.

zhaoqin · 2017-04-26T00:28:25Z


 int
-offline_instru_t::append_thread_header(byte *buf_ptr, thread_id_t tid)
+offline_instru_t::append_thread_file_header(byte *buf_ptr)


zhaoqin · 2017-04-26T00:30:17Z

-online_instru_t::append_thread_header(byte *buf_ptr, thread_id_t tid)
+online_instru_t::append_thread_file_header(byte *buf_ptr)
 {
    // The caller separately calls append_tid for us which is all we need.


zhaoqin · 2017-04-26T00:32:15Z

      torunonly_ci(tool.reuse ${ci_shared_app} drcachesim
        "reuse_distance.c" # for templatex basename
-        "-ipc_name drtestpipe6 -simulator_type reuse_distance -reuse_distance_threshold 256" "" "")
+        "-ipc_name drtestpipe6 -num_threads 0 -simulator_type reuse_distance -reuse_distance_threshold 256" "" "")


We had the thread pool test for drcachesim online simulation
Here I want to test the online simulation without threading pool, unless we do not need to support that any more.

zhaoqin · 2017-04-26T03:09:49Z

+        dr_global_alloc(op_num_threads.get_value() * sizeof(sideline_exit_events[0]));
+    for (i = 0; i < op_num_threads.get_value(); i++) {
+        sideline_exit_events[i] = dr_event_create();
+        sideline_args[i].id   = i;


zhaoqin · 2017-04-26T03:12:11Z

+        sideline_args[i].id   = i;
+        if (op_offline.get_value())
+            sideline_args[i].file = create_thread_file((ptr_int_t)i);
+        dr_create_client_thread(sideline_run, &sideline_args[i]);


zhaoqin · 2017-04-26T03:12:29Z

+    queue_get = 0;
+}
+
+void event_fork_init(void *drcontext)


zhaoqin · 2017-04-26T03:15:41Z

+        }
+        if (op_num_threads.get_value() > 1) {
+            NOTIFY(0, "Usage error: only one sideline thread is supported "
+                   "for online simulation\n");


done.
only one named pipe, so one thread makes sense, right?

zhaoqin · 2017-04-26T03:16:17Z

    redzone_size = instru->sizeof_entry() * MAX_NUM_ENTRIES;
    max_buf_size = trace_buf_size + redzone_size;
-    buf_hdr_slots_size = instru->sizeof_entry() * BUF_HDR_SLOTS;
+    /* get the header size by faking an unit header write */


zhaoqin · 2017-04-26T14:58:46Z

It probably will work but may lose accuracy, because more than one threads are writing to the pipe at the same time. I could remove the check

…

On Wed, Apr 26, 2017 at 12:28 AM, Derek Bruening ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In clients/drcachesim/tracer/tracer.cpp <#2319 (comment)>: > @@ -971,11 +1213,18 @@ drmemtrace_client_main(client_id_t id, int argc, const char *argv[]) droption_parser_t::usage_short(DROPTION_SCOPE_ALL).c_str()); dr_abort(); } - if (!op_offline.get_value() && op_ipc_name.get_value().empty()) { - NOTIFY(0, "Usage error: ipc name is required\nUsage:\n%s", - droption_parser_t::usage_short(DROPTION_SCOPE_ALL).c_str()); - dr_abort(); - } else if (op_offline.get_value() && op_outdir.get_value().empty()) { + if (!op_offline.get_value()) { + if (op_ipc_name.get_value().empty()) { + NOTIFY(0, "Usage error: ipc name is required\nUsage:\n%s", + droption_parser_t::usage_short(DROPTION_SCOPE_ALL).c_str()); + dr_abort(); + } + if (op_num_threads.get_value() > 1) { + NOTIFY(0, "Usage error: only one sideline thread is supported " + "for online simulation\n"); Will something not work correctly if there's more than one thread in the pool? Or it will work, you just think there's no perf improvement? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2319 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABkcDP-WKX2T3rlcNp-tERh3Sxx7PIvCks5rzsfcgaJpZM4Mu6lp> .

zhaoqin · 2017-04-26T15:26:37Z

No, still working on the corrrectness, saw some hangs.

…

On Wed, Apr 26, 2017 at 11:04 AM, Derek Bruening ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In clients/drcachesim/tracer/tracer.cpp <#2319 (comment)>: > } else { /* pass pid and tid to the simulator to register current thread */ - proc_info = (byte *)buf; - DR_ASSERT(BUFFER_SIZE_BYTES(buf) >= 3*instru->sizeof_entry()); - proc_info += instru->append_thread_header(proc_info, dr_get_thread_id(drcontext)); - proc_info += instru->append_tid(proc_info, dr_get_thread_id(drcontext)); - proc_info += instru->append_pid(proc_info, dr_get_process_id()); - write_trace_data(drcontext, (byte *)buf, proc_info); - - /* put buf_base to TLS plus header slots as starting buf_ptr */ - BUF_PTR(data->seg_base) = data->buf_base + buf_hdr_slots_size; + byte *buf = (byte *) + dr_raw_mem_alloc(max_buf_size, DR_MEMPROT_READ | DR_MEMPROT_WRITE, NULL); Did you measure the performance for online and for offline? We have some numbers for SPEC in the tracker and it would be good to see what effect the pool has on a simple app like that. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2319 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABkcDJ3KEU1q4IxHVxjTwOkSKuO4ktqHks5rz1zjgaJpZM4Mu6lp> .

zhaoqin · 2017-04-27T03:30:51Z

PTAL
there are some non-trivial changes, so it would be better to take another round of review.

zhaoqin · 2017-04-27T15:19:26Z

New commit message:
i#2006 generalize drcachesime: add threading pool support for writing out traces

Added threading pool for asynchronous trace writing out.

added two options for threading pool configuration,
used a circular buffer for producer-consumer queue,
added fork handling,
updated trace format to write trace per sideline thread instead of app thread,
updated raw2trace to handle the new trace format,
added new tests for different threading pool configurations,
removed handoff callbacks and corresponding tests.

derekbruening

Two main comments:

If this slows everything down it should be off by default until perf is improved
We need to add back thread and file footers for raw2trace: this change removes both thread and file truncation sanity checks

derekbruening · 2017-04-27T15:15:58Z

 "for later offline analysis.  No simulator is executed.");

+droption_t<unsigned int> op_num_threads
+(DROPTION_SCOPE_CLIENT, "num_threads", IF_WINDOWS_ELSE(0, 1),


This needs a comment: why is it different for Windows?

Also, if the pool is a perf hit (esp w/ only 1 thread) for offline (but we expect to make it a net win with tuning, right?), should it be disabled by default on all platforms to avoid slowing everyone down?

If this really depends on online vs offline then have the default set in C++ code and describe it in the docs here.

add comment

derekbruening · 2017-04-27T15:21:04Z

@@ -403,13 +403,13 @@ raw2trace_t::merge_and_process_thread_files()
    // The current thread we're processing is tidx.  If it's set to thread_files.size()


current file

I guess leaving as "tidx" is fine: less code churn.

derekbruening · 2017-04-27T15:22:26Z

                }
            }
-            VPRINT(2, "Next thread in timestamp order is %u @0x" ZHEX64_FORMAT_STRING
-                   "\n", (uint)tids[next_tidx], times[next_tidx]);


Why was this removed? I found it useful for seeing the operation: which file is next.

derekbruening · 2017-04-27T15:25:43Z

-                WARN("Input file for thread %d is truncated", (uint)tids[tidx]);
-                in_entry.extended.type = OFFLINE_TYPE_EXTENDED;
-                in_entry.extended.ext = OFFLINE_EXT_TYPE_FOOTER;
+                VPRINT(2, "Finish read file %d\n", tidx);


Finished reading

derekbruening · 2017-04-27T15:31:29Z

-                offline_entry_t entry;
-                if (thread_files[tidx]->read((char*)&entry, sizeof(entry)) ||
-                    !thread_files[tidx]->eof())
-                    FATAL_ERROR("Footer is not the final entry");


We're now missing two checks:

That each thread ends with a footer: this is important to know whether we had thread truncation. This was checked above at lines 458-462. I think we have to put this check back in (will have to iterate the threads at the end I guess), along with the warning and workaround.

That each file is not truncated. We probably need a new kind of footer now, a file footer entry.

derekbruening · 2017-04-27T15:33:45Z

+    buf_entry_t *entry;
+    do {
+        /* quick racy check without locking */
+        while (((queue_put + 1) & queue_size_mask) == queue_get) {


You said you saw major performance issues involving contention on the queue: please add comments about that so we know this code needs improvement.

derekbruening · 2017-04-27T15:35:40Z

          "${PROJECT_SOURCE_DIR}/clients/drcachesim/tests")
        set(tool.drcachesim.threads_rawtemp ON) # no preprocessor
-        set(tool.drcachesim.threads_timeout 150) # This test is long.
+        set(tool.drcachesim.threads_timeout 200) # This test is long.


The pool slows all of these down so much we need longer timeouts?? That's not good. Sounds like this should definitely be off by default until its performance is improved.

derekbruening · 2017-04-27T15:38:18Z

+write_trace_data(file_t file, void *data, ssize_t size)
+{
+    ssize_t written = file_ops_func.write_file(file, data, size);
+    DR_ASSERT(written == size);


This used to be NOTIFY0 with an understandable message and dr_abort, which seems better than "written==size" being printed

derekbruening · 2017-04-27T15:42:21Z

Also, the commit message should talk about the performance, since that's a big TODO item: reading the current commit message it sounds like this thread pool is all finished.

… out traces Added threading pool for asynchronous trace writing out. - added two options for threading pool configuration, - used a circular buffer for producer-consumer queue, - added fork handling, - updated trace format to write trace per sideline thread instead of app thread, - added a new offline ext type, - updated raw2trace to handle the new trace format, - added new tests for different threading pool configurations, - removed handoff callbacks and corresponding tests. TODO: the initial threading pool implementation caused a big slowdown on online simulation, we need more optimization and tuning to improve the performance.

zhaoqin

PTAL

zhaoqin · 2017-04-27T18:04:45Z

@@ -403,13 +403,13 @@ raw2trace_t::merge_and_process_thread_files()
    // The current thread we're processing is tidx.  If it's set to thread_files.size()


zhaoqin · 2017-04-27T18:06:45Z

                }
            }
-            VPRINT(2, "Next thread in timestamp order is %u @0x" ZHEX64_FORMAT_STRING
-                   "\n", (uint)tids[next_tidx], times[next_tidx]);


zhaoqin · 2017-04-27T18:07:23Z

-                WARN("Input file for thread %d is truncated", (uint)tids[tidx]);
-                in_entry.extended.type = OFFLINE_TYPE_EXTENDED;
-                in_entry.extended.ext = OFFLINE_EXT_TYPE_FOOTER;
+                VPRINT(2, "Finish read file %d\n", tidx);


zhaoqin · 2017-04-27T18:14:53Z

+write_trace_data(file_t file, void *data, ssize_t size)
+{
+    ssize_t written = file_ops_func.write_file(file, data, size);
+    DR_ASSERT(written == size);


zhaoqin · 2017-04-27T18:16:51Z

          "${PROJECT_SOURCE_DIR}/clients/drcachesim/tests")
        set(tool.drcachesim.threads_rawtemp ON) # no preprocessor
-        set(tool.drcachesim.threads_timeout 150) # This test is long.
+        set(tool.drcachesim.threads_timeout 200) # This test is long.


zhaoqin · 2017-04-27T18:29:55Z

+    buf_entry_t *entry;
+    do {
+        /* quick racy check without locking */
+        while (((queue_put + 1) & queue_size_mask) == queue_get) {


zhaoqin · 2017-04-27T20:48:36Z

-                offline_entry_t entry;
-                if (thread_files[tidx]->read((char*)&entry, sizeof(entry)) ||
-                    !thread_files[tidx]->eof())
-                    FATAL_ERROR("Footer is not the final entry");


zhaoqin · 2017-04-27T21:55:42Z

PTAL

derekbruening

Two cases where the current code has a nice message or a good check but this CL removes it: requesting reinstatement of the prior code for those two cases.

A question about the thread exit ordering which sounds like it may be a big deal. If I misunderstood and the exit is last then this shouldn't need another review.

derekbruening · 2017-04-28T03:08:28Z

+                VPRINT(2, "Finish reading file %d at pos %u\n",
+                       tidx, (int)thread_files[tidx]->tellg());
+                // We need do another failed read to make ->eof() return true.
+                if (thread_files[tidx]->read((char*)&in_entry, 1)) {


The previous code that you deleted seems better, "if (read entry || !eof) fatal..."

derekbruening · 2017-04-28T03:11:35Z

        }
    } while (file_count > 0);
+    // If we used threading pool during profiling, the trace for a thread may be
+    // seen after the thread exit entry, so we can only check whether the unique


What? But you can't put a thread exit entry in the final trace here and then put a memref entry for that thread afterward. Is that what's happening due to the timestamp changes? We have to enforce that order, don't we?

derekbruening · 2017-04-28T03:12:53Z

+    // tid and tid exit matches.
+    VPRINT(1, "%u thread(s) are seen\n", (uint)tid_set.size());
+    if (tid_set != tid_exit_set) {
+        WARN("Profile might be corrupted.");


A more specific message, please! Again the prior code was IMHO better: "Trace for thread %d is truncated".

derekbruening · 2017-04-28T03:13:40Z

        /* quick racy check without locking */
        while (((queue_put + 1) & queue_size_mask) == queue_get) {
-            /* XXX: use futex/semaphore for better performance. */
+            /* XXX: there is a big slowdown for the online simulation


I thought you said offline was slow too? You plan to put the numbers in the issue tracker, right?

… out traces Added threading pool for asynchronous trace writing out. - added two options for threading pool configuration, - used a circular buffer for producer-consumer queue, - added fork handling, - updated trace format to write trace per sideline thread instead of app thread, - updated raw2trace to handle the new trace format, - added new tests for different threading pool configurations.

i#2006 generalize drcachesime: add two options for asynchronous trace…

88e531a

… write Adds two options -num_threads and -queue_size to tracer for asynchronous trace write by sideline threading pool. This CL only adds the two options, real implementation is to be added.

derekbruening requested changes Mar 30, 2017

View reviewed changes

i#2006 generalize drcachesim: add two options for asynch trace write

4f3b1d5

Adds two options -num_threads and -queue_size to the tracer for asynchronous trace write by the sideline threads. This CL only adds the two options, and the real implementation is to be added.

zhaoqin and others added 4 commits March 30, 2017 22:05

i#2006 generalize drcachesim: add two options for asynch trace write

d5419b1

Adds two options -num_threads and -queue_size to the tracer for asynchronous trace write by the sideline threads. This CL only adds the two options, and the real implementation is to be added.

Merge branch 'master' into i2006-drcachesim-threadpool

ee5ee03

Fix Windows build

77e173a

derekbruening reviewed Apr 15, 2017

View reviewed changes

zhaoqin and others added 3 commits April 18, 2017 07:21

NOCHECKIN: add multi-sideline-thread support

f438042

Merge branch 'master' into i2006-drcachesim-threadpool

6824ec7

i#2006 generalize drcachesime: add threading pool

86d7a6b

Added threading pool for asynchronous trace dump. - used a circular buffer for producer consumer queue, - added fork handling, - updated trace format to write trace per sideline thread instead of app thread, - removed handoff callbacks.

zhaoqin added 2 commits April 24, 2017 21:08

add missing reset

7af1730

[NOCHECKIN] type cast to possible build warning

d6b3041

derekbruening changed the title ~~i#2006 generalize drcachesime: add two options for asynchronous trace…~~ i#2006 generalize drcachesim: add two options for asynchronous trace… Apr 25, 2017

zhaoqin added 4 commits April 24, 2017 23:10

Fix Windows build error

80cd649

remove sideline test option

29a7150

[NOCHECKIN] tweak windows test script

1d86dd0

fixed windows build error, enable/fix a synch online simulation test

3c7b019

derekbruening approved these changes Apr 25, 2017

View reviewed changes

i#2006 generalize drcachesime: add threading pool

36da490

Added threading pool for asynchronous trace dump. - used a circular buffer for producer consumer queue, - added fork handling, - updated trace format to write trace per sideline thread instead of app thread, - removed handoff callbacks.

zhaoqin commented Apr 26, 2017

View reviewed changes

zhaoqin added 2 commits April 25, 2017 23:23

Merge branch 'master' into i2006-drcachesim-threadpool

fd8f7a1

[NOCHECKIN] increase verbose

d7bb681

zhaoqin added 4 commits April 26, 2017 16:20

final

74485b5

tweak timeout and verbose

93d2efa

add more test

a7775c8

update raw2trace

eaff8c8

zhaoqin added 5 commits April 27, 2017 07:26

tweak verbose for build error

e3570e5

[NOCHECKIN] tweak again

8e3720a

fix build error

cb51855

Merge branch 'master' into i2006-drcachesim-threadpool

a8c1ff8

renable vs2010 build on AppVeyor

6167c0e

zhaoqin changed the title ~~i#2006 generalize drcachesim: add two options for asynchronous trace…~~ i#2006 generalize drcachesime: add threading pool support for writing out traces Apr 27, 2017

derekbruening requested changes Apr 27, 2017

View reviewed changes

derekbruening changed the title ~~i#2006 generalize drcachesime: add threading pool support for writing out traces~~ i#2006 generalize drcachesim: add threading pool support for writing out traces Apr 27, 2017

zhaoqin commented Apr 27, 2017

View reviewed changes

Merge branch 'master' into i2006-drcachesim-threadpool

6421857

derekbruening approved these changes Apr 28, 2017

View reviewed changes

zhaoqin added 5 commits April 28, 2017 11:26

Update two checks with more specific error message

cf8b527

fix windows build

bf1eb92

fix build error on raw2trace.cpp

1721536

fix windows build

5b1e9fb

zhaoqin mentioned this pull request May 3, 2017

improve memory trace performance #2001

Open

		@@ -403,13 +403,13 @@ raw2trace_t::merge_and_process_thread_files()
		// The current thread we're processing is tidx. If it's set to thread_files.size()

Uh oh!

Conversation

zhaoqin commented Mar 30, 2017

Uh oh!

derekbruening left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhaoqin commented Mar 31, 2017

Uh oh!

derekbruening left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekbruening left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment