8.68% of my time in assert()?
Turning off bounds checking for get and getRef does get rid of nearly all of the assert()s, but I was just trying -Winline and discovered: ./lout/misc.hh: In function 'void lout::misc::assert(bool, const char*, ...)': ./lout/misc.hh:36: warning: function 'void lout::misc::assert(bool, const char* , ...)' can never be inlined because it uses variable argument lists .. Earlier I'd said assert() was not cheap, but I was still uncertain about how -g and -pg might be affecting inlining. Now I _know_ that assert() is not cheap.
On Tue, Sep 16, 2008 at 05:41:04PM +0000, corvid wrote:
Turning off bounds checking for get and getRef does get rid of nearly all of the assert()s, but I was just trying -Winline and discovered:
./lout/misc.hh: In function 'void lout::misc::assert(bool, const char*, ...)': ./lout/misc.hh:36: warning: function 'void lout::misc::assert(bool, const char* , ...)' can never be inlined because it uses variable argument lists ..
Ouch. A short grep shows that variable argument lists are not used (yet) in the code - just const strings. So we could change assert to use a const char* argument if I'm not missing something.
Earlier I'd said assert() was not cheap, but I was still uncertain about how -g and -pg might be affecting inlining. Now I _know_ that assert() is not cheap.
Nice catch! Johannes
Johannes wrote:
On Tue, Sep 16, 2008 at 05:41:04PM +0000, corvid wrote:
Turning off bounds checking for get and getRef does get rid of nearly all of the assert()s, but I was just trying -Winline and discovered:
./lout/misc.hh: In function 'void lout::misc::assert(bool, const char*, ...)': ./lout/misc.hh:36: warning: function 'void lout::misc::assert(bool, const char* , ...)' can never be inlined because it uses variable argument lists ..
Ouch. A short grep shows that variable argument lists are not used (yet) in the code - just const strings. So we could change assert to use a const char* argument if I'm not missing something.
For me, grep shows layout.cc: misc::assert (anchor, "anchor '%s' not defined", name); misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d",
On Tue, Sep 16, 2008 at 07:20:31PM +0000, corvid wrote:
Johannes wrote:
On Tue, Sep 16, 2008 at 05:41:04PM +0000, corvid wrote:
Turning off bounds checking for get and getRef does get rid of nearly all of the assert()s, but I was just trying -Winline and discovered:
./lout/misc.hh: In function 'void lout::misc::assert(bool, const char*, ...)': ./lout/misc.hh:36: warning: function 'void lout::misc::assert(bool, const char* , ...)' can never be inlined because it uses variable argument lists ..
Ouch. A short grep shows that variable argument lists are not used (yet) in the code - just const strings. So we could change assert to use a const char* argument if I'm not missing something.
For me, grep shows layout.cc: misc::assert (anchor, "anchor '%s' not defined", name); misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d",
Must have missed these... In that case we could get away with something as in attached patch, but we should also consider to just switch to standard assert(). At the moment I would actually prefer the latter. What do others think? Cheers, Johannes PS: I don't see any speed difference with attached patch when loading the huge mysql manual page.
Johannes wrote:
On Tue, Sep 16, 2008 at 07:20:31PM +0000, corvid wrote:
Johannes wrote:
On Tue, Sep 16, 2008 at 05:41:04PM +0000, corvid wrote:
Turning off bounds checking for get and getRef does get rid of nearly all of the assert()s, but I was just trying -Winline and discovered:
./lout/misc.hh: In function 'void lout::misc::assert(bool, const char*, ...)': ./lout/misc.hh:36: warning: function 'void lout::misc::assert(bool, const char* , ...)' can never be inlined because it uses variable argument lists ..
Ouch. A short grep shows that variable argument lists are not used (yet) in the code - just const strings. So we could change assert to use a const char* argument if I'm not missing something.
For me, grep shows layout.cc: misc::assert (anchor, "anchor '%s' not defined", name); misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d",
Must have missed these... In that case we could get away with something as in attached patch, but we should also consider to just switch to standard assert(). At the moment I would actually prefer the latter. What do others think?
Standard assert() sounds fine to me.
On Tue, Sep 16, 2008 at 09:21:45PM +0000, corvid wrote:
Johannes wrote:
On Tue, Sep 16, 2008 at 07:20:31PM +0000, corvid wrote:
Johannes wrote:
On Tue, Sep 16, 2008 at 05:41:04PM +0000, corvid wrote:
Turning off bounds checking for get and getRef does get rid of nearly all of the assert()s, but I was just trying -Winline and discovered:
./lout/misc.hh: In function 'void lout::misc::assert(bool, const char*, ...)': ./lout/misc.hh:36: warning: function 'void lout::misc::assert(bool, const char* , ...)' can never be inlined because it uses variable argument lists ..
Ouch. A short grep shows that variable argument lists are not used (yet) in the code - just const strings. So we could change assert to use a const char* argument if I'm not missing something.
For me, grep shows layout.cc: misc::assert (anchor, "anchor '%s' not defined", name); misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d",
Must have missed these... In that case we could get away with something as in attached patch, but we should also consider to just switch to standard assert(). At the moment I would actually prefer the latter. What do others think?
Standard assert() sounds fine to me.
Ok. Here comes the patch. It also adds a missing check for negative values in SimpleVector methods. Cheers, Johannes
On Wed, Sep 17, 2008 at 05:11:08PM +0200, Johannes Hofmann wrote:
On Tue, Sep 16, 2008 at 09:21:45PM +0000, corvid wrote:
Johannes wrote:
On Tue, Sep 16, 2008 at 07:20:31PM +0000, corvid wrote:
Johannes wrote:
On Tue, Sep 16, 2008 at 05:41:04PM +0000, corvid wrote:
Turning off bounds checking for get and getRef does get rid of nearly all of the assert()s, but I was just trying -Winline and discovered:
./lout/misc.hh: In function 'void lout::misc::assert(bool, const char*, ...)': ./lout/misc.hh:36: warning: function 'void lout::misc::assert(bool, const char* , ...)' can never be inlined because it uses variable argument lists ..
Ouch. A short grep shows that variable argument lists are not used (yet) in the code - just const strings. So we could change assert to use a const char* argument if I'm not missing something.
For me, grep shows layout.cc: misc::assert (anchor, "anchor '%s' not defined", name); misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d",
Must have missed these... In that case we could get away with something as in attached patch, but we should also consider to just switch to standard assert(). At the moment I would actually prefer the latter. What do others think?
Standard assert() sounds fine to me.
Ok. Here comes the patch. It also adds a missing check for negative values in SimpleVector methods.
Does this patch show any performance improvement? If it does and has no portability problems, I'm for it too. -- Cheers Jorge.-
Jorge wrote:
On Wed, Sep 17, 2008 at 05:11:08PM +0200, Johannes Hofmann wrote:
On Tue, Sep 16, 2008 at 09:21:45PM +0000, corvid wrote:
Johannes wrote:
but we should also consider to just switch to standard assert(). At the moment I would actually prefer the latter. What do others think?
Standard assert() sounds fine to me.
Ok. Here comes the patch. It also adds a missing check for negative values in SimpleVector methods.
Does this patch show any performance improvement?
If it does and has no portability problems, I'm for it too.
Inlining succeeds now. 8% isn't enough to really notice, but gprof seems consistent with that time being saved. And we should probably rip out fail() as well so that no one ever makes the mistake of using it.
On Wed, Sep 17, 2008 at 05:34:12PM -0400, Jorge Arellano Cid wrote:
On Wed, Sep 17, 2008 at 05:11:08PM +0200, Johannes Hofmann wrote:
On Tue, Sep 16, 2008 at 09:21:45PM +0000, corvid wrote:
Johannes wrote:
On Tue, Sep 16, 2008 at 07:20:31PM +0000, corvid wrote:
Johannes wrote:
On Tue, Sep 16, 2008 at 05:41:04PM +0000, corvid wrote: > Turning off bounds checking for get and getRef does get rid of nearly > all of the assert()s, but I was just trying -Winline and discovered: > > ./lout/misc.hh: In function 'void lout::misc::assert(bool, const char*, ...)': > ./lout/misc.hh:36: warning: function 'void lout::misc::assert(bool, const char* > , ...)' can never be inlined because it uses variable argument lists > ..
Ouch. A short grep shows that variable argument lists are not used (yet) in the code - just const strings. So we could change assert to use a const char* argument if I'm not missing something.
For me, grep shows layout.cc: misc::assert (anchor, "anchor '%s' not defined", name); misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d",
Must have missed these... In that case we could get away with something as in attached patch, but we should also consider to just switch to standard assert(). At the moment I would actually prefer the latter. What do others think?
Standard assert() sounds fine to me.
Ok. Here comes the patch. It also adds a missing check for negative values in SimpleVector methods.
Does this patch show any performance improvement?
If it does and has no portability problems, I'm for it too.
I've checked the major BSDs and Linux seems to be fine too. According to http://en.wikipedia.org/wiki/C_standard_library it's part of the ANSI C library header and was introduced with C99. The same holds for stdint.h btw. for which we introduced some macro stuff in style.hh. I think we should simply rely on C99 compatible headers and include those without any conditionals. Cheers, Johannes
On Thu, Sep 18, 2008 at 06:29:33PM +0200, Johannes Hofmann wrote:
On Wed, Sep 17, 2008 at 05:34:12PM -0400, Jorge Arellano Cid wrote:
On Wed, Sep 17, 2008 at 05:11:08PM +0200, Johannes Hofmann wrote:
On Tue, Sep 16, 2008 at 09:21:45PM +0000, corvid wrote:
Johannes wrote:
On Tue, Sep 16, 2008 at 07:20:31PM +0000, corvid wrote:
Johannes wrote: > On Tue, Sep 16, 2008 at 05:41:04PM +0000, corvid wrote: > > Turning off bounds checking for get and getRef does get rid of nearly > > all of the assert()s, but I was just trying -Winline and discovered: > > > > ./lout/misc.hh: In function 'void lout::misc::assert(bool, const char*, ...)': > > ./lout/misc.hh:36: warning: function 'void lout::misc::assert(bool, const char* > > , ...)' can never be inlined because it uses variable argument lists > > .. > > Ouch. A short grep shows that variable argument lists are not used > (yet) in the code - just const strings. > So we could change assert to use a const char* argument if I'm not > missing something.
For me, grep shows layout.cc: misc::assert (anchor, "anchor '%s' not defined", name); misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d",
Must have missed these... In that case we could get away with something as in attached patch, but we should also consider to just switch to standard assert(). At the moment I would actually prefer the latter. What do others think?
Standard assert() sounds fine to me.
Ok. Here comes the patch. It also adds a missing check for negative values in SimpleVector methods.
Does this patch show any performance improvement?
If it does and has no portability problems, I'm for it too.
I've checked the major BSDs and Linux seems to be fine too. According to http://en.wikipedia.org/wiki/C_standard_library it's part of the ANSI C library header and was introduced with C99. The same holds for stdint.h btw. for which we introduced some macro stuff in style.hh. I think we should simply rely on C99 compatible headers and include those without any conditionals.
OK, let's try... - Assert patch committed. - Conditional switched to just include stdint.h. - fail() function removed. -- Cheers Jorge.-
On Thu, Sep 18, 2008 at 11:03:32PM -0400, Jorge Arellano Cid wrote:
On Thu, Sep 18, 2008 at 06:29:33PM +0200, Johannes Hofmann wrote:
On Wed, Sep 17, 2008 at 05:34:12PM -0400, Jorge Arellano Cid wrote:
On Wed, Sep 17, 2008 at 05:11:08PM +0200, Johannes Hofmann wrote:
On Tue, Sep 16, 2008 at 09:21:45PM +0000, corvid wrote:
Johannes wrote:
On Tue, Sep 16, 2008 at 07:20:31PM +0000, corvid wrote: > Johannes wrote: > > On Tue, Sep 16, 2008 at 05:41:04PM +0000, corvid wrote: > > > Turning off bounds checking for get and getRef does get rid of nearly > > > all of the assert()s, but I was just trying -Winline and discovered: > > > > > > ./lout/misc.hh: In function 'void lout::misc::assert(bool, const char*, ...)': > > > ./lout/misc.hh:36: warning: function 'void lout::misc::assert(bool, const char* > > > , ...)' can never be inlined because it uses variable argument lists > > > .. > > > > Ouch. A short grep shows that variable argument lists are not used > > (yet) in the code - just const strings. > > So we could change assert to use a const char* argument if I'm not > > missing something. > > For me, grep shows > layout.cc: misc::assert (anchor, "anchor '%s' not defined", name); > misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", > misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", > misc.hh: assert (i < this->num, "Array index out of bounds: %d >= %d", >
Must have missed these... In that case we could get away with something as in attached patch, but we should also consider to just switch to standard assert(). At the moment I would actually prefer the latter. What do others think?
Standard assert() sounds fine to me.
Ok. Here comes the patch. It also adds a missing check for negative values in SimpleVector methods.
Does this patch show any performance improvement?
If it does and has no portability problems, I'm for it too.
I've checked the major BSDs and Linux seems to be fine too. According to http://en.wikipedia.org/wiki/C_standard_library it's part of the ANSI C library header and was introduced with C99. The same holds for stdint.h btw. for which we introduced some macro stuff in style.hh. I think we should simply rely on C99 compatible headers and include those without any conditionals.
OK, let's try...
- Assert patch committed. - Conditional switched to just include stdint.h. - fail() function removed.
Great. I will fix things if this breaks something ;-) Cheers, Johannes
participants (3)
-
corvid＠lavabit.com -
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de