i#7216: noise generator basic structure #7283

edeiana · 2025-02-18T11:24:12Z

Scaffolding for the noise generator.
Adds noise_generator_t as a subclass of the iterator reader_t.
Currently noise_generator_t only produces TRACE_TYPE_READ records
with address 0xdeadbeef (easier to spot in tools' output). These records are
preceded by TRACE_TYPE_THREAD, TRACE_TYPE_PID,
TRACE_MARKER_TYPE_TIMESTAMP and followed by TRACE_TYPE_THREAD_EXIT
as this is the sequence of records the scheduler expects.
As of now, the user can add a single-process, single-thread noise generator workload to
the scheduler's input workloads through the -noise_generator_enable analyzer option
in the drmemtrace framework. Adding a noise_generator_t to the scheduler's input
workloads is done by the factory class noise_generator_factory_t through its
create_noise_generator() method, which returns a noise generator as an
input_reader_t that can then be added to the scheduler's input_workload_t
vector. Currently, there are three knobs the user can control in the noise generator:
PID, TID, and number of noise records to generate. These knobs are encapsulated
in the noise_generator_info_t struct. For now they are pre-set with default values:
PID = 1, TID = 1, number of noise records to generate = 1000.

Adds a unit test in scheduler_unit_tests.cpp and an end-to-end test leveraging
the schedule_stats tool.

Issue: #7216

Added -noise_generator_num_records to scheduler options info.

clients/drcachesim/common/options.cpp

clients/drcachesim/scheduler/scheduler_impl.cpp

clients/drcachesim/tests/schedule_stats_noise_generator.templatex

api/docs/release.dox

clients/drcachesim/common/options.cpp

clients/drcachesim/scheduler/noise_generator.cpp

clients/drcachesim/scheduler/scheduler_impl.cpp

clients/drcachesim/tests/scheduler_unit_tests.cpp

clients/drcachesim/scheduler/noise_generator.cpp

that satisfies the scheduler requirements (i.e., noise records must be wrapped between tid, pid, and thread exit). This improved code utilization between noise_generator_t and mock_noise_generator_t in the unit tests.

1.

clients/drcachesim/common/options.cpp

api/docs/release.dox

clients/drcachesim/scheduler/noise_generator.h

clients/drcachesim/scheduler/scheduler.h

clients/drcachesim/scheduler/scheduler_impl.cpp

Using defaults for pid, tid, and num_records. Moved addition of noise_generator_t to workloads in init_scheduler_common().

api/docs/release.dox

clients/drcachesim/scheduler/scheduler.h

clients/drcachesim/scheduler/noise_generator.h

clients/drcachesim/analyzer.cpp

clients/drcachesim/scheduler/noise_generator.h

Added timestamp. Removed dynamic_cast.

derekbruening · 2025-03-06T19:03:22Z

api/docs/release.dox

+ - Added #dynamorio::drmemtrace::noise_generator_t scaffolding as a
+   #dynamorio::drmemtrace::reader_t to produce synthetic trace records in the drmemtrace
+   framework.
+ - Added #dynamorio::drmemtrace::noise_generator_info_t, which contains the metadata


Make these all a nested list under a top level item on adding noise generator support?

derekbruening · 2025-03-06T19:04:13Z

clients/drcachesim/analyzer.cpp

+    // Add noise generator to workload_inputs.
+    if (noise_generator_factory_.is_enabled()) {
+        // TODO i#7216: here can be a good place to analyze the workloads in order to
+        // tweak  noise_generator_info_t parameters. We'd also need a method to replace


nit: Double space

Fixed, thanks!

derekbruening · 2025-03-06T19:05:36Z

clients/drcachesim/analyzer.cpp

+        std::string noise_generator_factory_error_string =
+            noise_generator_factory_.get_error_string();
+        if (!noise_generator_factory_error_string.empty()) {
+            error_string_ = noise_generator_factory_error_string;


Just assign this instead of using the local var? If we don't want to clobber a pre-existing error: this is already clobbering it; could append.

Done (with append).

derekbruening · 2025-03-06T19:11:38Z

clients/drcachesim/scheduler/noise_generator.cpp

+noise_generator_factory_t<trace_entry_t, record_reader_t>::create_noise_generator(
+    addr_t pid, addr_t tid, uint64_t num_records)
+{
+    error_string_ = "Noise generator is not suppported for record_reader_t";


We'll need this to create core-sharded traces via record_filter. Include a TODO or XXX so we know we plan to implement this. Or implement it now.

Added comment. I prefer to break it down, this PR is already on the large side.

derekbruening · 2025-03-06T19:17:00Z

clients/drcachesim/scheduler/noise_generator.h

+    get_error_string();
+
+    void
+    add_noise_generator_to_workloads(


This seems an inflexible approach: it doesn't allow adding a noise thread within a an existing process, or a set of noise processes each with different thread counts, or a set of noise threads each with different noise characteristics (load/store mix, etc.). This interface does not seem like it will work for future uses.

I'm going to stick with my original suggestion at https://github.com/DynamoRIO/dynamorio/pull/7283/files#r1973978208 of a factory that takes in a noise_generator_info_t and returns a single input_reader_t. That gives the most flexibility.

Another layer or maybe another method in the same factory could provide convenience support for creating a set of N noise generator threads/processes, but the user can do that through iteration and has full control over the config of each. I guess non-colliding tids/pids is a factor: but this PR doesn't solve that either if the factory is used twice. Maybe the factory for a single one increments an atomic integer to use for the id.

a factory that takes in a noise_generator_info_t and returns a single input_reader_t. That gives the most flexibility.

Agreed. Done.
noise_generator_info_t now controls a single noise_generator_t for maximum flexibility.
create_noise_generator() in noise_generator_factory_t takes a noise_generator_info_t and returns a single-process, single-thread noise_generator_t as an input_reader_t.

Another layer or maybe another method in the same factory could provide convenience support for creating a set of N noise generator threads/processes, but the user can do that through iteration and has full control over the config of each.

Since we don't have a clear use for that right now, I removed the add_noise_generator_to_workloads() convenience method. The user is currently responsible for non-colliding PIDs (I think we can have colliding TIDs, as long as they belong to different processes).

derekbruening · 2025-03-06T19:26:38Z

clients/drcachesim/scheduler/noise_generator.h

+    init();
+
+    bool
+    is_enabled();


This "enabled" seems odd: it's not used to gate the add_ function; it is in fact ignored by the internal code of this class. It doesn't seem like it belongs inside the class.

Right, moved to the analyzer instead.

in the form of an input_reader_t, for max flexibility.

test.

we oonly care about threads anyway.

record in the noise generator.

derekbruening · 2025-03-10T15:42:07Z

clients/drcachesim/analyzer.cpp

+        typename sched_type_t::input_reader_t noise_generator_reader =
+            noise_generator_factory_.create_noise_generator(noise_generator_info);
+        // Check for errors.
+        error_string_ = error_string_ + noise_generator_factory_.get_error_string();


+= should work

derekbruening · 2025-03-10T15:43:37Z

clients/drcachesim/analyzer.cpp

+    if (noise_generator_enabled_) {
+        // TODO i#7216: here can be a good place to analyze the workloads in order to
+        // tweak noise_generator_info_t parameters. For now we use noise_generator_info_t
+        // default values.


Also comment that for now we create a single noise thread but we might want more in the future?

derekbruening · 2025-03-10T15:46:02Z

clients/drcachesim/common/options.cpp

+    op_noise_generator_enable(DROPTION_SCOPE_FRONTEND, "noise_generator_enable", false,
+                              "Enables noise generation.",
+                              "Enables the scheduler to interleave trace records with "
+                              "synthetic records produced by a noise generator.");


This description is not clear: it could be interpreted as not actually adding any noise but just enabling someone else to add noise. Make it clear that it adds actual noise inputs. Maybe say right now it adds a single input but that may change in the future, to give us room to auto-add more threads/processes or add other CLI options as we decide in the future.

Maybe even change the name to add_noise_generator instead of "enable"? That seems clearer.

derekbruening · 2025-03-10T15:48:28Z

clients/drcachesim/scheduler/noise_generator.h

+    addr_t pid = 1;
+    addr_t tid = 1;
+    uint64_t num_records_to_generate = 1000;
+};


Add a TODO or XXX about adding parameters controlling the mix of noise: load/store mix, address patterns, etc., so it's clear we don't consider this to be the final set of option fields.

derekbruening · 2025-03-10T15:48:54Z

clients/drcachesim/scheduler/noise_generator.h

+ * Contains metadata information to drive the noise generation.
+ */
+struct noise_generator_info_t {
+    noise_generator_info_t() {};


Use = default;

derekbruening · 2025-03-10T16:12:46Z

clients/drcachesim/tests/scheduler_unit_tests.cpp

+        std::unique_ptr<mock_noise_generator_t>(new mock_noise_generator_t(
+            noise_generator_info, noise_generator_addr_to_generate)),
+        std::unique_ptr<mock_noise_generator_t>(new mock_noise_generator_t()),
+        INVALID_THREAD_ID);


Shouldn't this match the info tid? Needs a named constant?

derekbruening · 2025-03-10T16:13:22Z

clients/drcachesim/tests/scheduler_unit_tests.cpp

+    std::vector<scheduler_t::input_workload_t> sched_inputs;
+    sched_inputs.emplace_back(std::move(readers));
+    if (scheduler.init(sched_inputs, 1,
+                       scheduler_t::make_scheduler_serial_options(/*verbosity=*/4)) !=


Serial shouldn't be supported as the max timestamps will put the noise at the very end.
You could do the same as serial but pass DEPENDENCY_IGNORE (hmm would have to review the scheduler to see what order it would do then).

derekbruening · 2025-03-10T16:14:34Z

clients/drcachesim/tests/scheduler_unit_tests.cpp

+        std::unique_ptr<mock_noise_generator_t>(new mock_noise_generator_t(
+            noise_generator_info, noise_generator_addr_to_generate)),
+        std::unique_ptr<mock_noise_generator_t>(new mock_noise_generator_t()),
+        INVALID_THREAD_ID);


Please add a test of using the factory. Seems ok to change this test to use the factory instead?

derekbruening · 2025-03-10T16:18:37Z

clients/drcachesim/tests/scheduler_unit_tests.cpp

+        // There is just one output so we expect to always see 0 as the ordinal.
+        assert(stream->get_input_workload_ordinal() == 0);
+        // All TRACE_TYPE_READ records are generated by the noise generator and their
+        // addr is always the constant noise_generator_addr_to_generate.


You don't need to do this arrangement with the only reads in the noise: you have the tid of the noise input, and you also have the ordinal of the noise input. You can use one of those, and then put reads in the other inputs, which seems a stronger check.

derekbruening · 2025-03-10T16:20:25Z

clients/drcachesim/tests/schedule_stats_noise_generator.templatex

+Schedule stats tool results:
+Total counts:
+           *[1-9][0-9]* cores
+           2 threads:.*


The small_trace_file IIRC is asm with a fixed known count of record types, and other tests rely on those? So should we check that the result has small_trace_file read count plus 100? Is 100 hidden from us?

edeiana added 12 commits February 17, 2025 23:59

Save work.

d426b35

Save work.

f445f78

Save work.

f70f5f5

Minor improvements.

3814809

Save work.

cbe2176

Cleanup.

9f1a73c

Fixed result aggregation in scheduler issue.

d99c991

Do not count tid, pid, and tid exit when generating synthetic records.

e143e2f

Moved noise_generator into scheduler dir.

db8aa84

Added noise_generator_num_records flag and noise_generator unit test.

03d79a2

Minor improvements.

2e70cd4

Added end-to-end schedule stats test with noise_generator.

ae2d44f

edeiana changed the title ~~i#7216 noise generator basic structure~~ i#7216: noise generator basic structure Feb 21, 2025

edeiana added 2 commits February 21, 2025 03:10

Added changes to doc.

c906bf7

Added -noise_generator_num_records to scheduler options info.

Removed references from doc, as doxygen refuses to generate a link.

d7a4365

edeiana commented Feb 21, 2025

View reviewed changes

clients/drcachesim/common/options.cpp Outdated Show resolved Hide resolved

edeiana commented Feb 21, 2025

View reviewed changes

clients/drcachesim/scheduler/scheduler_impl.cpp Outdated Show resolved Hide resolved

edeiana marked this pull request as ready for review February 21, 2025 11:38

edeiana requested a review from derekbruening February 21, 2025 11:38

Cannot rely on thread numbers or order for schedule_stats output.

6e21191

edeiana commented Feb 21, 2025

View reviewed changes

clients/drcachesim/tests/schedule_stats_noise_generator.templatex Outdated Show resolved Hide resolved

derekbruening reviewed Feb 21, 2025

View reviewed changes

edeiana added 7 commits February 21, 2025 18:24

Added get_noise_generator_end().

d1a9047

Seprate logic to generate noise records from the logic

ddc8e49

that satisfies the scheduler requirements (i.e., noise records must be wrapped between tid, pid, and thread exit). This improved code utilization between noise_generator_t and mock_noise_generator_t in the unit tests.

Add multiple noise generators with -noise_generator_add.

d823067

Fixed dr_option type of -noise_generator_add from bool to uint64_t.

09e2a0d

End-to-end noise generator test now using 3 noise generators instead of

04fabbc

1.

Updated doc.

fddaecd

Add default value of 0 to pid and tid of noise_generator_t.

9fcf581

edeiana requested a review from derekbruening February 24, 2025 14:04

derekbruening reviewed Feb 24, 2025

View reviewed changes

edeiana added 8 commits February 26, 2025 19:39

Removed noise_generator global flags, except -noise_generator_enable.

ca8a792

Using defaults for pid, tid, and num_records. Moved addition of noise_generator_t to workloads in init_scheduler_common().

Added noise_Generator_info_t and fixed doc.

382230e

Removed relative path to trace_entry.h include.

21c8216

Merge branch 'master' into i7216-noise-generator-initial

863a3be

noise_generator_info is now a unique ptr in scheduler options.

212cefb

clang format.

e62c3c2

Missing include added.

5a0a4a6

Fixed typo in comment.

9a22912

edeiana requested a review from derekbruening February 27, 2025 13:22

derekbruening reviewed Feb 27, 2025

View reviewed changes

edeiana added 5 commits March 6, 2025 02:03

Added noise_generator_factory_t.

91c0b2e

Added timestamp. Removed dynamic_cast.

Ensure schedule_stats_noise_generator always simulates on 4 cores.

ec872b0

Report error if adding noise generators fails.

410c108

Improved comment.

45a6841

Improved comment.

b3dc6de

edeiana requested a review from derekbruening March 6, 2025 11:42

derekbruening reviewed Mar 6, 2025

View reviewed changes

edeiana added 6 commits March 7, 2025 18:02

noise_generator_factory_t only generates a single noise generator

1bf889c

in the form of an input_reader_t, for max flexibility.

Trying to schedule always on 4 cores in schedule_stats_noise_generator

6a9a4d3

test.

Ignore core count in schedule_stats_noise_generator test,

2223afc

we oonly care about threads anyway.

Minor improvements.

4509a31

Improved noise generator scheduler unit test.

a344710

Minor improvements.

1a35d61

edeiana requested a review from derekbruening March 9, 2025 09:41

edeiana added 2 commits March 9, 2025 01:46

Typo in comment fixed.

4c746e0

Clarification comment on why we need a timestamp

3c4968f

record in the noise generator.

derekbruening approved these changes Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i#7216: noise generator basic structure #7283

i#7216: noise generator basic structure #7283

edeiana commented Feb 18, 2025 •

edited

Loading

derekbruening Mar 6, 2025

edeiana Mar 9, 2025

derekbruening Mar 6, 2025

edeiana Mar 9, 2025

derekbruening Mar 6, 2025

edeiana Mar 9, 2025

derekbruening Mar 6, 2025

edeiana Mar 9, 2025 •

edited

Loading

derekbruening Mar 6, 2025

edeiana Mar 9, 2025

derekbruening Mar 6, 2025

edeiana Mar 9, 2025

derekbruening Mar 10, 2025

derekbruening Mar 10, 2025

derekbruening Mar 10, 2025

derekbruening Mar 10, 2025

derekbruening Mar 10, 2025

derekbruening Mar 10, 2025

derekbruening Mar 10, 2025

derekbruening Mar 10, 2025

derekbruening Mar 10, 2025

derekbruening Mar 10, 2025

derekbruening Mar 10, 2025

i#7216: noise generator basic structure #7283

Are you sure you want to change the base?

i#7216: noise generator basic structure #7283

Conversation

edeiana commented Feb 18, 2025 • 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

edeiana Mar 9, 2025 • 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

edeiana commented Feb 18, 2025 •

edited

Loading

edeiana Mar 9, 2025 •

edited

Loading