Cleanup obsolete debugging code

Details

Description

The current Diags.h D/EClosure mechanism is obsolete. ATS requires gcc >= 4.1 for all compilation environments, and it includes variadic argument macro support with ##VA_ARGS that deletes the final comma if no arguments are provided.
Removing the added layer should also improve performance when high volume debugging is turned on.

Uri Shachar
added a comment - 05/Feb/12 09:49 New patch created against the current trunk. Not sure why the previous patch was failing on your machine – it applied cleanly + regression passed on my setup.

Leif Hedstrom
added a comment - 05/Feb/12 17:50 I think the old SVN repo is "read-only" since the migration happened. The git repo is at
http://git-wip-us.apache.org/repos/asf/trafficserver.git
(or https:// if you want a read-write repo, the http:// is read-only).
I'll look at the rejections in a bit, but if you have time, prepping one that patches against git trunk (master) would be great.

Could it be that we aren't compiling with gcc (Seems like it: -errof isn't a gcc flag)?
The patch assumes gcc usage (which is what the build wiki page specifies), I can probably get it working on solaris as well with some ifdefs - but it won't look as good...

Uri Shachar
added a comment - 08/Feb/12 09:01 Could it be that we aren't compiling with gcc (Seems like it: -errof isn't a gcc flag)?
The patch assumes gcc usage (which is what the build wiki page specifies), I can probably get it working on solaris as well with some ifdefs - but it won't look as good...

Leif Hedstrom
added a comment - 08/Feb/12 14:12 Yeah, sorry. What it really means is that we don't support e.g gcc 3.x and earlier. We still want to support as many "modern" compilers as possible, eg Intel icc and clang/llvm.
If you could, fixing this would be great.

Uri Shachar
added a comment - 08/Feb/12 14:48 clang/llvm claim to support this out of the box.
Intel icc as well, though it might issue a warning about not being strict C99 compatible (depending on compiler version).
Solaris has #_ VA_ARGS _ which does exactly the same thing (Missing the additional # just to be different ), and might also require a "#pragma error_messages (off, E_ARGUEMENT_MISMATCH)".
I'll generate a patch for solaris compiler, but I don't have access to a solaris machine to test it.
Could you verify that the other build-bots pass ok before I get to work on this?

Yeah, all other build-bots are happy. And yes, icc is generally "gcc 4.x" compatible, and I think llvm tries to be as well. Solaris is well, Solaris, so no big surprise there ...

I have a Solaris build box I can test it on as well, but, this is why we have the buildbots in the first place. I'll probably spend some time next to get both icc and llvm to compile again, and then we should aim to get buildbots for those as well.

Thanks for taking the time to work on this, we really appreciate it, and of course, we welcome all new (and old committers to work with the project.

Leif Hedstrom
added a comment - 08/Feb/12 14:53 Yeah, all other build-bots are happy. And yes, icc is generally "gcc 4.x" compatible, and I think llvm tries to be as well. Solaris is well, Solaris, so no big surprise there ...
I have a Solaris build box I can test it on as well, but, this is why we have the buildbots in the first place. I'll probably spend some time next to get both icc and llvm to compile again, and then we should aim to get buildbots for those as well.
Thanks for taking the time to work on this, we really appreciate it, and of course, we welcome all new (and old committers to work with the project.

Leif Hedstrom
added a comment - 16/Feb/12 21:44 We might need to back this out. Not only is it breaking Solaris builds, it actually segfaults the OSX gcc-llvm compiler (not our fault obviously, but still annoying). Uri, what do you think ?

Took me ages to set up a solaris dev env, and then I saw your latest commit – it looks a lot better then the bunch of ifdefs I had going.
I'll add another patch soon to remove the redundant DebugOn command, and then (finally) go back to the txn specific debugging functionality.

Uri Shachar
added a comment - 19/Feb/12 16:57 Took me ages to set up a solaris dev env, and then I saw your latest commit – it looks a lot better then the bunch of ifdefs I had going.
I'll add another patch soon to remove the redundant DebugOn command, and then (finally) go back to the txn specific debugging functionality.

I'm doing some benchmarks (I've tested both with and without --disable-diags), and from what I can tell, the 1102 commits makes the server about 15% slower... I'm surprised that compiling with --disable-diags doesn't work around that, but I'm 100% certain it's slower.

15% degradation is definitely unacceptable (I expected a slight improvement) – and "--disable-diags" should show no change at all... Do you test with any tags turned on?
I'll do some benchmarks on my side and get back to you today.

Uri Shachar
added a comment - 23/Feb/12 10:03 15% degradation is definitely unacceptable (I expected a slight improvement) – and "--disable-diags" should show no change at all... Do you test with any tags turned on?
I'll do some benchmarks on my side and get back to you today.

First thing first: --disable-diags does not do anything (it sets TS_USE_DIAGS to 0 but Diags.h tests if is defined or not)....

I haven't been able to reproduce anything close to a 15% degradation, but i have seen 1.7%. Looks like the increased overhead of always calling the multiple argument function outweighs everything else. When I do:
-#define Diag(tag, fmt, ...) diags>log(tag, DTA(DL_Diag), fmt, ##VA_ARGS)
+#define Diag(tag, fmt, ...) if (diags->on()) diags->log(tag, DTA(DL_Diag), fmt, ##VA_ARGS)

We get the slight perf boost I expected when a tag is turned on, and no impact when diags is turned off.
Attaching a patch (with the DebugOn removal) + the benchmark results that I got.

Uri Shachar
added a comment - 23/Feb/12 14:23 First thing first: --disable-diags does not do anything (it sets TS_USE_DIAGS to 0 but Diags.h tests if is defined or not)....
I haven't been able to reproduce anything close to a 15% degradation, but i have seen 1.7%. Looks like the increased overhead of always calling the multiple argument function outweighs everything else. When I do:
-#define Diag(tag, fmt, ...) diags>log(tag, DTA(DL_Diag), fmt, ## VA_ARGS )
+#define Diag(tag, fmt, ...) if (diags->on()) diags->log(tag, DTA(DL_Diag), fmt, ## VA_ARGS )
We get the slight perf boost I expected when a tag is turned on, and no impact when diags is turned off.
Attaching a patch (with the DebugOn removal) + the benchmark results that I got.