Messages in this thread | | | Date | Mon, 30 Mar 2015 17:00:45 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH v2 2/2] perf tool: Fix ppid for synthesized fork events |
| |
Em Mon, Mar 30, 2015 at 01:53:04PM -0600, David Ahern escreveu: > On 3/30/15 1:40 PM, Arnaldo Carvalho de Melo wrote: > >>@@ -81,6 +84,7 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) > >> > >> name = strstr(bf, "Name:"); > >> tgids = strstr(bf, "Tgid:"); > >>+ ppids = strstr(bf, "PPid:"); > > > >can't we make this: > > > > ppids = strstr(tgids, "PPid:"); > > > >To speed it up a teeny little bit? 8-) > > Sure, I thought about that as well, but it puts an assumption on order of > the data in the file. Why have the assumption?
Good question, perhaps we should leave it as is, should be in the noise, right?
> >> if (name) { > >> name += 5; /* strlen("Name:") */ > >>@@ -109,32 +113,51 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len) > >> if (nl) > >> *nl = '\0'; > >> > >>- tgid = atoi(tgids); > >>+ *tgid = atoi(tgids); > >> > >> } else > >> pr_debug("Tgid: string not found for pid %d\n", pid); > >> > >>- return tgid; > >>+ if (ppids) { > >>+ ppids += 5; /* strlen("PPid:") */ > >>+ > >>+ while (*ppids && isspace(*ppids)) > >>+ ++ppids; > > > >The above could be simplified to: > > > > while (isspace(*ppids)) > > ++ppids; > > sure.
And then, the this loop can be elided as well, as you can see below...
> >$ cat isspace.c > >#include <ctype.h> > >#include <stdio.h> > >int main(void) { return printf("isspace('\\0')=%d\n", isspace('\0')); } > >$ ./isspace > >isspace('\0')=0 > >$ > > > >>+ nl = strchr(ppids, '\n'); > >>+ if (nl) > >>+ *nl = '\0'; > > > >We also don't need to find and zero this '\n', as: > > > >$ cat atoi.c > >#include <string.h> > >#include <stdio.h> > >int main(void) { return printf("atoi(\"1234\\n\")=%d\n", > >atoi("1234\n")); } > >$ ./atoi > >atoi("1234\n")=1234 > >$ > > ok. another assumption on implementation. fine with taking it out.
Not really, POSIX sayz:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/atoi.html
The call atoi(str) shall be equivalent to:
(int) strtol(str, (char **)NULL, 10)
----
And:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html
These functions shall convert the initial portion of the string pointed to by str to a type long and long long representation, respectively. First, they decompose the input string into three parts:
An initial, possibly empty, sequence of white-space characters (as specified by isspace())
A subject sequence interpreted as an integer represented in some radix determined by the value of base
A final string of one or more unrecognized characters, including the terminating NUL character of the input string.
----------
So even that isspace loop can be ditched, no?
> ><SNIP> > > > >>+ if (machine__is_host(machine)) { > >>+ if (perf_event__get_comm_ids(pid, event->comm.comm, > >>+ sizeof(event->comm.comm), > >>+ tgid, ppid) != 0) { > >>+ return -1; > >>+ } > >>+ } else > >>+ *tgid = machine->pid; > > > >Somebody, I think PeterZ and also Ingo, routinely asks for having {} on > >the else part of an if that has {}, please do so. > > ack
Thanks!
- Arnaldo
| |