[v3] Optimize record/replay checkpointing for all clocks it applies to

Removes redundant checkpoints in replay log when there are no expired timers in timers list, associated with corresponding clock (i.e. no rr events associated with current clock value).
This also improves performance in rr mode.

Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
---

Oops, forgot to commit this fix

    v3:
    - fixed compiler warning caused non-debug build to fail

 include/qemu/timer.h |  2 +-
 util/qemu-timer.c    | 62 +++++++++++++++++++++++++---------------------------
 2 files changed, 31 insertions(+), 33 deletions(-)

On 18/10/2018 13:16, Artem Pisarenko wrote:
> Removes redundant checkpoints in replay log when there are no expired timers in timers list, associated with corresponding clock (i.e. no rr events associated with current clock value).
> This also improves performance in rr mode.
> 
> Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
> ---
> 
> Oops, forgot to commit this fix
> 
>     v3:
>     - fixed compiler warning caused non-debug build to fail

We can also move the switch statement to a separate function, it
simplifies the code:

diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 8a2ad3bce2..3a64ce33d3 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -482,6 +482,26 @@ bool timer_expired(QEMUTimer *timer_head, int64_t
current_time)
     return timer_expired_ns(timer_head, current_time * timer_head->scale);
 }

+static bool timer_checkpoint(QEMUClockType clock)
+{
+    if (replay_mode != REPLAY_MODE_NONE) {
+        switch (clock) {
+        case QEMU_CLOCK_VIRTUAL:
+            return replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL);
+        case QEMU_CLOCK_HOST:
+            return replay_checkpoint(CHECKPOINT_CLOCK_HOST);
+        case QEMU_CLOCK_VIRTUAL_RT:
+            return replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL_RT);
+        default:
+            /* QEMU_CLOCK_REALTIME is external to the emulation and does
+             * not need checkpointing.
+             */
+            break;
+        }
+    }
+    return true;
+}
+
 bool timerlist_run_timers(QEMUTimerList *timer_list)
 {
     QEMUTimer *ts;
@@ -489,8 +509,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
     bool progress = false;
     QEMUTimerCB *cb;
     void *opaque;
-    bool need_replay_checkpoint = false;
-    ReplayCheckpoint replay_checkpoint_id;
+    bool need_replay_checkpoint = true;

     if (!atomic_read(&timer_list->active_timers)) {
         return false;
@@ -501,28 +520,6 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
         goto out;
     }

-    if (replay_mode != REPLAY_MODE_NONE) {
-        /* Postpone actual checkpointing to timer list processing
-         * to properly check if we actually need it.
-         */
-        switch (timer_list->clock->type) {
-        case QEMU_CLOCK_VIRTUAL:
-            need_replay_checkpoint = true;
-            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL;
-            break;
-        case QEMU_CLOCK_HOST:
-            need_replay_checkpoint = true;
-            replay_checkpoint_id = CHECKPOINT_CLOCK_HOST;
-            break;
-        case QEMU_CLOCK_VIRTUAL_RT:
-            need_replay_checkpoint = true;
-            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL_RT;
-            break;
-        default:
-            break;
-        }
-    }
-
     /*
      * Extract expired timers from active timers list and and process them,
      * taking into account checkpointing required in rr mode.
@@ -545,11 +542,11 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
             break;
         }
         if (need_replay_checkpoint
                 && !(ts->attributes & QEMU_TIMER_ATTR_EXTERNAL)) {
             /* once we got here, checkpoint clock only once */
             need_replay_checkpoint = false;
             qemu_mutex_unlock(&timer_list->active_timers_lock);
-            if (!replay_checkpoint(replay_checkpoint_id)) {
+            if (!timer_checkpoint(timer_list->clock->type)) {
                 goto out;
             }
             qemu_mutex_lock(&timer_list->active_timers_lock);


No need to do anything on your part.

Paolo

> We can also move the switch statement to a separate function, it
> simplifies the code:
> ...

When I prepared this patch my intuition said me to add note in advance:
"Paolo, please, don't try to move this to a separate function. I've tried
it already. It cannot be done correct, look nice and not decrease
performancy at the same time". But I've ignored it... :)
Change you did is correct and nice, but (compared to my version) it adds
extra unlock/lock pair for running each timer list when it isn't empty and
in non-rr mode (where we would ignore checkpoints and execution flow would
bypass whole "if (need_replay_checkpoint) {...}" block).
Maybe you're aware of it, but I don't think that such change worth it.

On 18/10/2018 15:23, Artem Pisarenko wrote:
>> We can also move the switch statement to a separate function, it
>> simplifies the code:
>> ...
> 
> When I prepared this patch my intuition said me to add note in advance:
> "Paolo, please, don't try to move this to a separate function. I've
> tried it already. It cannot be done correct, look nice and not decrease
> performancy at the same time". But I've ignored it... :)
> Change you did is correct and nice, but (compared to my version) it adds
> extra unlock/lock pair for running each timer list when it isn't empty
> and in non-rr mode (where we would ignore checkpoints and execution flow
> would bypass whole "if (need_replay_checkpoint) {...}" block).
> Maybe you're aware of it, but I don't think that such change worth it.

No, you're right.  The if should remain in the caller, or
need_replay_checkpoint must be initialized with replay_mode.

Paolo

> чт, 18 окт. 2018 г., 20:31 Paolo Bonzini:
>On 18/10/2018 15:23, Artem Pisarenko wrote:
>>> We can also move the switch statement to a separate function, it
>>> simplifies the code:
>>> ...
>>
>> When I prepared this patch my intuition said me to add note in advance:
>> "Paolo, please, don't try to move this to a separate function. I've
>> tried it already. It cannot be done correct, look nice and not decrease
>> performancy at the same time". But I've ignored it... :)
>> Change you did is correct and nice, but (compared to my version) it adds
>> extra unlock/lock pair for running each timer list when it isn't empty
>> and in non-rr mode (where we would ignore checkpoints and execution flow
>> would bypass whole "if (need_replay_checkpoint) {...}" block).
>> Maybe you're aware of it, but I don't think that such change worth it.
>
> No, you're right. The if should remain in the caller, or
> need_replay_checkpoint must be initialized with replay_mode.

If initialize 'need_replay_checkpoint', then it should also account for
clock != QEMU_CLOCK_REALTIME. And here we come to what if+switch block
actually (mostly) does in my version. Finally, you will get duplication of
this whole condition usage between source function and extracted function,
which isn't nice.
Why do you want to split up such tightly coupled code?

On 18/10/2018 19:10, Artem Pisarenko wrote:
> 
>> No, you're right. The if should remain in the caller, or
>> need_replay_checkpoint must be initialized with replay_mode.
> 
> If initialize 'need_replay_checkpoint', then it should also account for
> clock != QEMU_CLOCK_REALTIME.

Or you just get a unlock/lock pair for QEMU_CLOCK_REALTIME (which should
really never happen if e.g. you have no UI).

> And here we come to what if+switch block
> actually (mostly) does in my version. Finally, you will get duplication
> of this whole condition usage between source function and extracted
> function, which isn't nice.
> Why do you want to split up such tightly coupled code?

Because it's *too* coupled and not very readable.

Paolo

>чт, 18 окт. 2018 г., 23:25 Paolo Bonzini:
>On 18/10/2018 19:10, Artem Pisarenko wrote:
>>
>>> No, you're right. The if should remain in the caller, or
>>> need_replay_checkpoint must be initialized with replay_mode.
>>
>> If initialize 'need_replay_checkpoint', then it should also account for
>> clock != QEMU_CLOCK_REALTIME.
>
> Or you just get a unlock/lock pair for QEMU_CLOCK_REALTIME (which should
> really never happen if e.g. you have no UI).

And still have duplication (just smaller in this case).


>> Why do you want to split up such tightly coupled code?
>
> Because it's *too* coupled and not very readable.

Tightly coupled code in my understanding is property having its roots in
design, which usually has wider context than piece of code in question. So
we canot avoid this by definition (limiting changes only to this code).
Trying to improve it is just like a playing with bubble wrap - pressing
each bubble causes another bubble to pop up.

In our particular case it's because of overall rr design, qemu architecture
and a way how rr integrated in timers. The best we can do in this case is
to localise/quarantine ugly aspects as much as possible, carefully and
plenteously comment and try don't touch them... never... I consider
'timerlist_run_timers()' is already totally infected and we're just late to
save anyone of its residents (by isolating others).