| Message ID | 20170725143923.11241-1-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > >
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
"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?
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.
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?
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
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);
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(-)