Compute and pring the time that it takes to execute system calls. This data...

Compute and pring the time that it takes to execute system calls. This data
is going to be skewed slightly, as calling gettimeofday() by itself also
takes a little bit of time. But it should be good enough to allow us to see
where we have performance bottlenecks.

TEST=none
BUG=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41905

[Evan Martin, 2010-03-17 17:24:38]

I can't review the asm bits, I think you need agl.

http://codereview.chromium.org/997009/diff/1/10
File sandbox/linux/seccomp/debug.h (right):

http://codereview.chromium.org/997009/diff/1/10#newcode63
sandbox/linux/seccomp/debug.h:63: static inline bool enter();
Does inline actually work if the code isn't available here?

http://codereview.chromium.org/997009/diff/1/5
File sandbox/linux/seccomp/exit.cc (right):

http://codereview.chromium.org/997009/diff/1/5#newcode12
sandbox/linux/seccomp/exit.cc:12: Debug::syscall(&tm, __NR_exit, "Executing
handler");
Do you intentionally not call elapsed here?

http://codereview.chromium.org/997009/diff/1/15
File sandbox/linux/seccomp/syscall.cc (right):

http://codereview.chromium.org/997009/diff/1/15#newcode283
sandbox/linux/seccomp/syscall.cc:283: Debug::syscall(&tm,syscallNum, "Allowing
unrestricted system call");
space after comma

[Markus (顧孟勤), 2010-03-17 19:30:38]

I was so hoping that I could get Evan to review this change, as I don't want to
use up my "codereview-credits" with Adam. Not sure how many are left in my
account, now that he no longer officially works on Chrome.

But yes, if you don't feel comfortable with it, then Adam is going to be on the
hook for this one.

Pretty please :-)

http://codereview.chromium.org/997009/diff/1/10
File sandbox/linux/seccomp/debug.h (right):

http://codereview.chromium.org/997009/diff/1/10#newcode63
sandbox/linux/seccomp/debug.h:63: static inline bool enter();
This code is only going to be referenced from debug.cc, even though it is
declared here. It is my understanding, and I might very well be wrong, that I
need to request inlining in the declaration of the function, rather than in the
definition.

On the other hand, this might all be moot anyway. "inline" is only a hint, and
gcc has gotten more and more aggressive about ignoring it altogether and instead
relying on its on heuristics.

So, if you feel strongly about it, I'd be OK with remove the "inline" from the
header.

http://codereview.chromium.org/997009/diff/1/5
File sandbox/linux/seccomp/exit.cc (right):

http://codereview.chromium.org/997009/diff/1/5#newcode12
sandbox/linux/seccomp/exit.cc:12: Debug::syscall(&tm, __NR_exit, "Executing
handler");
On 2010/03/17 17:24:38, Evan Martin wrote:
> Do you intentionally not call elapsed here?

We can't really do much here. We'll never return from exit() (unless things went
horribly wrong). So, there is no way for us to print a real estimate for elapsed
time.

I could print a fake number that is always going to be 0ms. But that seemed
somewhat silly. Do you think, I should do that here?

http://codereview.chromium.org/997009/diff/1/15
File sandbox/linux/seccomp/syscall.cc (right):

http://codereview.chromium.org/997009/diff/1/15#newcode283
sandbox/linux/seccomp/syscall.cc:283: Debug::syscall(&tm,syscallNum, "Allowing
unrestricted system call");
Done

[agl, 2010-03-17 19:59:46]

LGTM.

(p.s. I haven't left Chrome team in any useful sense so feel free to send CLs my
way :)

http://codereview.chromium.org/997009/diff/22001/15020
File sandbox/linux/seccomp/debug.cc (right):

http://codereview.chromium.org/997009/diff/22001/15020#newcode133
sandbox/linux/seccomp/debug.cc:133: asm volatile("mov  %%gs, %0\n"
Why test for GS == 0? Can we be running without TLS? (Worth a comment.)

Later: ok, I see why. Certainly worth a comment.

http://codereview.chromium.org/997009/diff/22001/15020#newcode228
sandbox/linux/seccomp/debug.cc:228: *tm = 1000*1000*tv.tv_sec + tv.tv_usec;
You know the rules better than I here, but I worry about overflow. tv.tv_sec is
just a time_t so I worry that the multiplication is done in that type.

http://codereview.chromium.org/997009/diff/22001/15020#newcode245
sandbox/linux/seccomp/debug.cc:245: itoa(const_cast<char *>(strrchr(sysname =
unnamed, '\000')), sysnum);
Pick whichever you like, but you could save the strrchr:

static const char kUnnamedMessage[] = "Unnamed syscall #";

char buffer[40];
memcpy(buffer, kUnnamedMessage, sizeof(kUnnamedMessage) - 1);
itoa(buffer + sizeof(kUnnamedMessage) - 1, sysnum)

http://codereview.chromium.org/997009/diff/22001/15020#newcode281
sandbox/linux/seccomp/debug.cc:281: static const char *extra = "";
static const char extra[1] = {0};  <- saves a relocation record.

http://codereview.chromium.org/997009/diff/22001/15010
File sandbox/linux/seccomp/debug.h (right):

http://codereview.chromium.org/997009/diff/22001/15010#newcode63
sandbox/linux/seccomp/debug.h:63: static inline bool enter();
I think inline is worthless here:

"GCC does not inline any functions when not optimizing unless you specify the
`always_inline' attribute for the function, like this" [1]

When optimising, GCC will apply the usual heuristics for inlining anway, and
probably does a better-than-human job at it.

[1] http://gcc.gnu.org/onlinedocs/gcc-4.4.3/gcc/Inline.html

http://codereview.chromium.org/997009/diff/22001/15010#newcode72
sandbox/linux/seccomp/debug.h:72: static int  numSyscallNames_;
Missing alignment space before "num..." here?