[v2,1/1] monitor: improve tracing in handle_qmp_command
diff mbox

Message ID 20170725143923.11241-1-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev July 25, 2017, 2:39 p.m. UTC
Calculate req_json only if trace_handle_qmp_command enabled.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Lluís Vilanova <vilanova@ac.upc.edu>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
---
Changes from v1:
- written in the explicit for, as discussed in the mailing list

 monitor.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Eric Blake July 25, 2017, 2:44 p.m. UTC | #1
On 07/25/2017 09:39 AM, Denis V. Lunev wrote:
> Calculate req_json only if trace_handle_qmp_command enabled.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lluís Vilanova <vilanova@ac.upc.edu>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---
> Changes from v1:
> - written in the explicit for, as discussed in the mailing list

I could live with this being considered a bug-fix for 2.10, as we
regressed in speed/memory usage during normal untraced QMP commands.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
>  monitor.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index d8ac20f6ca..2bfeb9bbcc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3822,7 +3822,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      QDict *qdict = NULL;
>      Monitor *mon = cur_mon;
>      Error *err = NULL;
> -    QString *req_json;
>  
>      req = json_parser_parse_err(tokens, NULL, &err);
>      if (!req && !err) {
> @@ -3840,9 +3839,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          qdict_del(qdict, "id");
>      } /* else will fail qmp_dispatch() */
>  
> -    req_json = qobject_to_json(req);
> -    trace_handle_qmp_command(mon, qstring_get_str(req_json));
> -    qobject_decref(QOBJECT(req_json));
> +    if (trace_event_get_state(TRACE_HANDLE_QMP_COMMAND)) {
> +        QString *req_json = qobject_to_json(req);
> +        trace_handle_qmp_command(mon, qstring_get_str(req_json));
> +        qobject_decref(QOBJECT(req_json));
> +    }
>  
>      rsp = qmp_dispatch(cur_mon->qmp.commands, req);
>  
>
Stefan Hajnoczi July 28, 2017, 9:15 a.m. UTC | #2
On Tue, Jul 25, 2017 at 05:39:23PM +0300, Denis V. Lunev wrote:
> Calculate req_json only if trace_handle_qmp_command enabled.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lluís Vilanova <vilanova@ac.upc.edu>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---
> Changes from v1:
> - written in the explicit for, as discussed in the mailing list
> 
>  monitor.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index d8ac20f6ca..2bfeb9bbcc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3822,7 +3822,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      QDict *qdict = NULL;
>      Monitor *mon = cur_mon;
>      Error *err = NULL;
> -    QString *req_json;
>  
>      req = json_parser_parse_err(tokens, NULL, &err);
>      if (!req && !err) {
> @@ -3840,9 +3839,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          qdict_del(qdict, "id");
>      } /* else will fail qmp_dispatch() */
>  
> -    req_json = qobject_to_json(req);
> -    trace_handle_qmp_command(mon, qstring_get_str(req_json));
> -    qobject_decref(QOBJECT(req_json));
> +    if (trace_event_get_state(TRACE_HANDLE_QMP_COMMAND)) {
> +        QString *req_json = qobject_to_json(req);
> +        trace_handle_qmp_command(mon, qstring_get_str(req_json));
> +        qobject_decref(QOBJECT(req_json));
> +    }
>  
>      rsp = qmp_dispatch(cur_mon->qmp.commands, req);

Thanks for the patch.  Another fix is needed first so that SystemTap and
LTTng UST users can use this trace event.  Currently
trace_event_get_state(TRACE_HANDLE_QMP_COMMAND) will return false
because SystemTap and LTTng UST maintain their own state.  I have CCed
you on the fix.

Stefan
Markus Armbruster July 28, 2017, 5:01 p.m. UTC | #3
"Denis V. Lunev" <den@openvz.org> writes:

> Calculate req_json only if trace_handle_qmp_command enabled.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lluís Vilanova <vilanova@ac.upc.edu>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---
> Changes from v1:
> - written in the explicit for, as discussed in the mailing list
>
>  monitor.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index d8ac20f6ca..2bfeb9bbcc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3822,7 +3822,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      QDict *qdict = NULL;
>      Monitor *mon = cur_mon;
>      Error *err = NULL;
> -    QString *req_json;
>  
>      req = json_parser_parse_err(tokens, NULL, &err);
>      if (!req && !err) {
> @@ -3840,9 +3839,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          qdict_del(qdict, "id");
>      } /* else will fail qmp_dispatch() */
>  
> -    req_json = qobject_to_json(req);
> -    trace_handle_qmp_command(mon, qstring_get_str(req_json));
> -    qobject_decref(QOBJECT(req_json));
> +    if (trace_event_get_state(TRACE_HANDLE_QMP_COMMAND)) {
> +        QString *req_json = qobject_to_json(req);
> +        trace_handle_qmp_command(mon, qstring_get_str(req_json));
> +        qobject_decref(QOBJECT(req_json));
> +    }
>  
>      rsp = qmp_dispatch(cur_mon->qmp.commands, req);

Conflicts with Eric's commit 8a4613a, but I can resolve that in my tree.

The commit message is too terse.  "Improve tracing" makes me think of
more informative traces, but that's not the case.  What exactly is
improved here?
Eric Blake July 28, 2017, 6:14 p.m. UTC | #4
On 07/28/2017 12:01 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
> 
>> Calculate req_json only if trace_handle_qmp_command enabled.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Lluís Vilanova <vilanova@ac.upc.edu>
>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> ---

> Conflicts with Eric's commit 8a4613a, but I can resolve that in my tree.
> 
> The commit message is too terse.  "Improve tracing" makes me think of
> more informative traces, but that's not the case.  What exactly is
> improved here?

Reduce overhead when not tracing.  Without the patch, we are malloc'ing
a QString and spending CPU cycles on converting a QObject to string,
just for the sake of sticking the string in the trace message - if we
aren't tracing, it's wasted effort.
Markus Armbruster July 31, 2017, 7:25 a.m. UTC | #5
Eric Blake <eblake@redhat.com> writes:

> On 07/28/2017 12:01 PM, Markus Armbruster wrote:
>> "Denis V. Lunev" <den@openvz.org> writes:
>> 
>>> Calculate req_json only if trace_handle_qmp_command enabled.
>>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: Lluís Vilanova <vilanova@ac.upc.edu>
>>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> ---
>
>> Conflicts with Eric's commit 8a4613a, but I can resolve that in my tree.
>> 
>> The commit message is too terse.  "Improve tracing" makes me think of
>> more informative traces, but that's not the case.  What exactly is
>> improved here?
>
> Reduce overhead when not tracing.  Without the patch, we are malloc'ing
> a QString and spending CPU cycles on converting a QObject to string,
> just for the sake of sticking the string in the trace message - if we
> aren't tracing, it's wasted effort.

What about:

    monitor: Reduce handle_qmp_command() tracing overhead

    We are malloc'ing a QString and spending CPU cycles on converting a
    QObject to string, just for the sake of sticking the string in the
    trace message.  Wasted when we aren't tracing.  Avoid that.

Denis?
Stefan Hajnoczi Aug. 1, 2017, 10:04 a.m. UTC | #6
On Tue, Jul 25, 2017 at 05:39:23PM +0300, Denis V. Lunev wrote:
> Calculate req_json only if trace_handle_qmp_command enabled.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lluís Vilanova <vilanova@ac.upc.edu>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---
> Changes from v1:
> - written in the explicit for, as discussed in the mailing list
> 
>  monitor.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Replaced commit message & description with the one Markus suggested to
include more details on why this patch is necessary.

Thanks, applied to my tracing tree:
https://github.com/stefanha/qemu/commits/tracing

Stefan

Patch
diff mbox

diff --git a/monitor.c b/monitor.c
index d8ac20f6ca..2bfeb9bbcc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3822,7 +3822,6 @@  static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     QDict *qdict = NULL;
     Monitor *mon = cur_mon;
     Error *err = NULL;
-    QString *req_json;
 
     req = json_parser_parse_err(tokens, NULL, &err);
     if (!req && !err) {
@@ -3840,9 +3839,11 @@  static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         qdict_del(qdict, "id");
     } /* else will fail qmp_dispatch() */
 
-    req_json = qobject_to_json(req);
-    trace_handle_qmp_command(mon, qstring_get_str(req_json));
-    qobject_decref(QOBJECT(req_json));
+    if (trace_event_get_state(TRACE_HANDLE_QMP_COMMAND)) {
+        QString *req_json = qobject_to_json(req);
+        trace_handle_qmp_command(mon, qstring_get_str(req_json));
+        qobject_decref(QOBJECT(req_json));
+    }
 
     rsp = qmp_dispatch(cur_mon->qmp.commands, req);