Skip to content

Commit

Permalink
i#6721: Read ahead in as-traced drmemtrace mode
Browse files Browse the repository at this point in the history
Removes the exception for avoiding reading ahead to get the version
and filetype for as-traced drmemtrace mode, as we need those values
for the record_filter.

Adds explicit error checking and error return up front to the
record_filter for missing version info.

Adjusts the scheduler unit test's record ordinal to account for the
new read-ahead.

Adds a test with a checked-in trace of hello,world (truncated to make
it smaller) that ran on 4 different cores.  The test only reproduces
the problem if the read-ahead in open_reader() to find the tid is
removed, which I did temporarily; I kept the test as it is also a good
additional general test of other corner cases.  To create a test that
always reproduces in our suite we would need to combine a synthetic
reader with use of the scheduler in as-traced mode, which none of our
unit tests are set up for.

The fix was further tested on large inputs in an internal setup which
always passes a synthetic reader, where the problem reproduces every
time without the fix.

Fixes #6721
  • Loading branch information
derekbruening committed Mar 26, 2024
1 parent 781f15e commit bad26f7
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 6 deletions.
15 changes: 10 additions & 5 deletions clients/drcachesim/scheduler/scheduler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -809,10 +809,6 @@ scheduler_tmpl_t<RecordType, ReaderType>::set_initial_schedule(
// The filetype, if present, is before the first timestamp. If we only need the
// filetype we avoid going as far as the timestamp.
bool gather_filetype = options_.read_inputs_in_init;
// Avoid reading ahead for replay as it makes the input ords not match in tests.
if (options_.mapping == MAP_TO_RECORDED_OUTPUT &&
options_.replay_as_traced_istream != nullptr)
gather_filetype = false;
if (gather_filetype || gather_timestamps) {
sched_type_t::scheduler_status_t res =
get_initial_input_content(gather_timestamps);
Expand Down Expand Up @@ -1043,7 +1039,11 @@ scheduler_tmpl_t<RecordType, ReaderType>::read_recorded_schedule()
}
for (int i = 0; i < static_cast<output_ordinal_t>(outputs_.size()); ++i) {
if (outputs_[i].record.empty()) {
// XXX i#6630: We should auto-set the output count and avoid
// having extra outputs; these complicate idle computations, etc.
VPRINT(this, 1, "output %d empty: returning eof up front\n", i);
set_cur_input(i, INVALID_INPUT_ORDINAL);
outputs_[i].at_eof = true;
} else if (outputs_[i].record[0].type == schedule_record_t::IDLE) {
set_cur_input(i, INVALID_INPUT_ORDINAL);
outputs_[i].waiting = true;
Expand Down Expand Up @@ -1205,8 +1205,13 @@ scheduler_tmpl_t<RecordType, ReaderType>::read_traced_schedule()
outputs_[output_idx].record[0].key.input);
set_cur_input(output_idx, outputs_[output_idx].record[0].key.input);
}
} else
} else {
// XXX i#6630: We should auto-set the output count and avoid
// having extra ouputs; these complicate idle computations, etc.
VPRINT(this, 1, "Output %d empty: returning eof up front\n", output_idx);
outputs_[output_idx].at_eof = true;
set_cur_input(output_idx, INVALID_INPUT_ORDINAL);
}
}
return STATUS_SUCCESS;
}
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
7 changes: 7 additions & 0 deletions clients/drcachesim/tests/record_filter_start_idle.templatex
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Output .* entries from .* entries.
Schedule stats tool results:
.*
Core #0 schedule: .*
Core #1 schedule: .*
Core #2 schedule: .*
Core #3 schedule: .*
5 changes: 4 additions & 1 deletion clients/drcachesim/tests/scheduler_unit_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3423,7 +3423,10 @@ test_replay_as_traced_from_file(const char *testdir)
static const char *const SCHED_STRING =
"Core #0: 1257598 \nCore #1: 1257603 \nCore #2: 1257601 \n"
"Core #3: 1257599 => 1257604 @ <366987,87875,13331862029895453> "
"(<366986,87875,13331862029895453> => <1,0,0>) \n"
// The ordinal is really 1 ("<1,0,0>") but with the scheduler's readahead
// it becomes 2; easier to just check for that as trying to avoid readahead
// causes other problems (i#xxxx).
"(<366986,87875,13331862029895453> => <2,0,0>) \n"
"Core #4: 1257600 \nCore #5: 1257596 \nCore #6: 1257602 \n";
static constexpr int NUM_OUTPUTS = 7; // Matches the actual trace's core footprint.
scheduler_t scheduler;
Expand Down
21 changes: 21 additions & 0 deletions clients/drcachesim/tools/filter/record_filter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,10 @@ record_filter_t::initialize_shard_output(per_shard_t *per_shard,
// Now synchronize determining the extension.
auto lock = std::unique_lock<std::mutex>(input_info_mutex_);
if (!output_ext_.empty()) {
VPRINT(this, 2,
"Shard #%d using pre-set ext=%s, ver=%" PRIu64 ", type=%" PRIu64 "\n",
shard_stream->get_shard_index(), output_ext_.c_str(), version_,
filetype_);
per_shard->output_path += output_ext_;
lock.unlock();
} else if (!input_name.empty()) {
Expand All @@ -218,12 +222,25 @@ record_filter_t::initialize_shard_output(per_shard_t *per_shard,
// Set the other key input data.
version_ = shard_stream->get_version();
filetype_ = add_to_filetype(shard_stream->get_filetype());
if (version_ == 0) {
// We give up support for version 0 to have an up-front error check
// rather than having some output files with bad headers (i#6721).
return "Version not available at shard init time";
}
VPRINT(this, 2,
"Shard #%d setting ext=%s, ver=%" PRIu64 ", type=%" PRIu64 "\n",
shard_stream->get_shard_index(), output_ext_.c_str(), version_,
filetype_);
per_shard->output_path += output_ext_;
lock.unlock();
input_info_cond_var_.notify_all();
} else {
// We have to wait for another shard with an input to set output_ext_.
input_info_cond_var_.wait(lock, [this] { return !output_ext_.empty(); });
VPRINT(this, 2,
"Shard #%d waited for ext=%s, ver=%" PRIu64 ", type=%" PRIu64 "\n",
shard_stream->get_shard_index(), output_ext_.c_str(), version_,
filetype_);
per_shard->output_path += output_ext_;
lock.unlock();
}
Expand Down Expand Up @@ -643,6 +660,10 @@ record_filter_t::process_delayed_encodings(per_shard_t *per_shard, trace_entry_t
bool
record_filter_t::parallel_shard_memref(void *shard_data, const trace_entry_t &input_entry)
{
if (!success_) {
// Report an error that happened during shard init.
return false;
}
per_shard_t *per_shard = reinterpret_cast<per_shard_t *>(shard_data);
++per_shard->input_entry_count;
trace_entry_t entry = input_entry;
Expand Down
18 changes: 18 additions & 0 deletions suite/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4696,6 +4696,24 @@ if (BUILD_CLIENTS)
set(tool.record_filter_as_traced_basedir
"${PROJECT_SOURCE_DIR}/clients/drcachesim/tests")
set(tool.record_filter_as_traced_rawtemp ON) # no preprocessor

# Test the record_filter in as-traced mode with start-idle cores.
# This checked-in partial-hello,world-trace ran on 4 different cores.
set(trace_dir
"${PROJECT_SOURCE_DIR}/clients/drcachesim/tests/drmemtrace.as_traced.x64.tracedir")
set(sched_file "${trace_dir}/cpu_schedule.bin.zip")
set(outdir ${CMAKE_CURRENT_BINARY_DIR}/filter_start_idle)
file(MAKE_DIRECTORY ${outdir})
torunonly_api(tool.record_filter_start_idle "${drcachesim_path}"
"record_filter_start_idle"
"" "-simulator_type;schedule_stats;-indir;${outdir}" OFF OFF)
set(tool.record_filter_start_idle_runcmp "${CMAKE_CURRENT_SOURCE_DIR}/runmulti.cmake")
set(tool.record_filter_start_idle_precmd
"${drcachesim_path}@-simulator_type@record_filter@-cpu_schedule_file@${sched_file}@-core_sharded@-cores@4@-indir@${trace_dir}@-outdir@${outdir}")
set(tool.record_filter_start_idle_basedir
"${PROJECT_SOURCE_DIR}/clients/drcachesim/tests")
set(tool.record_filter_start_idle_rawtemp ON) # no preprocessor

endif ()

if (AARCH64)
Expand Down

0 comments on commit bad26f7

Please sign in to comment.