config: Add "events" key in parse_task_phase_data() #108

douglas-raillard-arm · 2021-02-04T16:25:51Z

Allow parsing an "events" key for phases with the following format:

    "events": [
	{"run": 1000},
	{"timer": {"ref": "helloworld", "period": 16000}},
	{"run": 9000},
	{"sleep": 1000}
    ]

This allows:

Enforced ordering of events. Relying on the order of keys in an
object is issue-prone, as interfaces to interact with mappings in
most languages are not designed around preserving such order. On
the contrary, array operations have always a well defined
order semantic.
More critically, this allows repeating the same event an
arbitrary number of times. Complex workload might otherwise need
to be split among different phases to workaround that issue, which
comes with some overhead and extra bookeeping in external code.

Checked constraints:

only event objects are allowed in the array
objects can only have one key
when "events" is in use, no event key is allowed in the phase
body. This means the user either uses the old format or the new
one but mixing is not allowed.

Signed-off-by: Douglas RAILLARD [email protected]

Allow parsing an "events" key for phases with the following format: "events": [ {"run": 1000}, {"timer": {"ref": "helloworld", "period": 16000}}, {"run": 9000}, {"sleep": 1000} ] This allows: * Enforced ordering of events. Relying on the order of keys in an object is issue-prone, as interfaces to interact with mappings in most languages are not designed around preserving such order. On the contrary, array operations have always a well defined order semantic. * More critically, this allows repeating the same event an arbitrary number of times. Complex workload might otherwise need to be split among different phases to workaround that issue, which comes with some overhead and extra bookeeping in external code. Checked constraints: * only event objects are allowed in the array * objects can only have one key * when "events" is in use, no event key is allowed in the phase body. This means the user either uses the old format or the new one but mixing is not allowed. Signed-off-by: Douglas RAILLARD <[email protected]>

vingu-linaro · 2021-03-10T08:52:38Z

doc/tutorial.txt

+
+  Each object in the array must contain a single event key. This format allows
+  specifying the same event more than once (like "run" in the example), and is
+  friendlier to languages with map types that are not preserving insertion


This is also the case with current mode if you read the beg of the file

Do you mean via workgen or the way rt-app handles things currently ?

vingu-linaro · 2021-03-10T08:53:28Z

doc/tutorial.txt

@@ -650,7 +674,7 @@ The generated events are of these main categories:

  - rtapp_loop: event=start thread_loop=0 phase=0 phase_loop=0
  - rtapp_loop: event=end thread_loop=0 phase=0 phase_loop=0
-  
+


please don't mix typo and new feature in the same patch or PR

Ah looks like my text editor stripped the lines, sorry about that

vingu-linaro · 2021-03-10T08:53:56Z

doc/tutorial.txt

@@ -756,5 +780,3 @@ below)
   20000 ++--+----+---+----+---+---+----+---+----+--++ 494000
       17560556057560556057560656065606756065606756075607
                    Loop start time [msec]
-


vingu-linaro · 2021-03-10T13:41:14Z

src/rt-app_parse_config.c


 	log_info(PFX "Parsing phase");

+	data->nbevents = 0;
+	data->events = NULL;


why do you need this ?

data->events=NULL is probably an artifact from before I introduced events_array variable, it's indeed not necessary anymore

vingu-linaro · 2021-03-10T13:41:47Z

src/rt-app_parse_config.c


 	log_info(PFX "Parsing phase");

+	data->nbevents = 0;
+	data->events = NULL;


Please try to not move code unnecessarily

vingu-linaro · 2021-03-10T13:55:13Z

src/rt-app_parse_config.c

 	foreach(obj, entry, key, val, idx) {
-		if (obj_is_event(key))
+		if(!strncmp("events", key, 6)) {


All this code seems overly complicated and transforms a simple straight forward function in something really complex.; From 2 foreach, it now needs 5 foreach/for loops

events_obj = get_in_object(obj, "events", TRUE); will say you if there is an events array or not

Indeed, I was looking for a function like that in json-c but it turns out it's a helper in that file.

vingu-linaro · 2021-03-10T14:48:21Z

src/rt-app_parse_config.c

+	foreach(obj, entry, key, val, idx) {
+		if (obj_is_event(key)) {
+			/* Check that we are not trying to mix old and new styles */
+			if (events_array) {


if an events array is detected we should simply ignore old style

Is it really an improvement though ? The foreach is still needed to count the number of events anyway and silently ignoring events is not really something users would thank us for.

EDIT: count the number of old-style event*

but when we have found an array we can skip this counting entirely because we don't care about old style events

My point is that this code cannot be removed, if someone is using old-style, this foreach needs to be somewhere. Silently ignoring part of the configuration file in order to save one iteration over a map that has a few tens of keys at most does not sound like a good deal. Even more considered the fact that in well-formed JSON files, the number of keys is bounded by the number of settings, so the only ones "paying a performance cost" are people writing a conf file that is ambiguous and will result in an error. (bearing in mind, we are parsing a configuration file, ahead of time of execution, in C, with an order of magnitude of a few tens or hundreds of keys in extreme cases).

On top of that, this can allow in the future to reclaim some event names for some settings. This sounds a bit crazy given that they have "verb" names for the most part, but the current syntax is lenient: we only match the prefix. This means that something like "synchronous-io" key is currently impossible to add, but would be possible with the "events" array as long as we forbid events outside of it when in use.

vingu-linaro · 2021-03-10T15:04:15Z

src/rt-app_parse_config.c

+		for (i=0; i < data->nbevents; i++) {
+			array_val = json_object_array_get_idx(events_array, i);
+			is_first_entry = true;
+			foreach(array_val, entry, key, val, idx) {


what is the purpose of this foreach ?
isn't all object inside the array an event

The array items are themselves objects:

[{event1: param1}, {event2: param2}, ...]

The alternative would be to use sub-arrays like a tuple instead of objects, but the object approach is the standard way of doing things, as it is pretty clean in YAML:

events: - run: 42 - sleep: 53

(and the sub-array solution does not save any typing anyway)

but that was already the case with the old style and obj_is_event() is there to check that those objects are events

After checking parse_task_event_data() it looks like it's doing a validation on the event name and will exit if it's given a non-event so we can skip this check here.

We still need to check the number of keys though, as:

0 keys would mean that the length of the array (and therefore the size of data->events) does not match the actual number of events

more than one key does not make sense (at least for now, maybe one day this could be used to attach some sort of metadata to the event)

I need to fix the code to check we are not in the 0 keys situation, currently it would trigger a UB with data->events[i] not being initialized.

douglas-raillard-arm force-pushed the events-phase-key branch 3 times, most recently from d3c54a4 to 31f57fb Compare February 4, 2021 17:32

douglas-raillard-arm force-pushed the events-phase-key branch from 31f57fb to ee562d0 Compare February 4, 2021 17:35

douglas-raillard-arm mentioned this pull request Feb 5, 2021

broken examples #110

Open

vingu-linaro reviewed Mar 10, 2021

View reviewed changes

		@@ -650,7 +674,7 @@ The generated events are of these main categories:

		- rtapp_loop: event=start thread_loop=0 phase=0 phase_loop=0
		- rtapp_loop: event=end thread_loop=0 phase=0 phase_loop=0

config: Add "events" key in parse_task_phase_data() #108

Are you sure you want to change the base?

config: Add "events" key in parse_task_phase_data() #108

Uh oh!

Conversation

douglas-raillard-arm commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

douglas-raillard-arm Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

douglas-raillard-arm commented Feb 4, 2021 •

edited

Loading

douglas-raillard-arm Mar 10, 2021 •

edited

Loading