Opened 8 years ago

Closed 7 years ago

#2236 closed task (fixed)

Add a --enable-debug configure flag

Reported by: vorner Owned by: muks
Priority: low Milestone: Sprint-20121106
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 1.74 Internal?: no

Description

Add a --enable-debug configure switch. The purpose is to turn on some debugging utilities that are too expensive for deployment but may help development or finding problems.

One of such thighs is enabling/disabling PTHREAD_MUTEX_ERRORCHECK and PTHREAD_MUTEX_ROBUST in src/lib/util/threads/lock.cc from #2202. Currently it unconditionally enables such checks, which might be needlessly slow.

Subtickets

Change History (14)

comment:1 Changed 7 years ago by vorner

  • Milestone set to Next-Sprint-Proposed

The #2202 had indeed some performance impact, much of which probably is the debug aid in it. It would be nice to add this soon to get the full speed of the software.

comment:2 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20121023

comment:3 Changed 7 years ago by jelte

  • Priority changed from medium to low

comment:4 Changed 7 years ago by muks

  • Owner set to muks
  • Status changed from new to assigned

Picking as there are no review bugs.

comment:5 Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

This bug is ready for review.

Note that config.h is included in sync.h, and some methods are conditionally available. This effectively means sync.h cannot be dist-ed. If we want to change this, we'll have to make the stuff in sync.h and conditionally add their definitions inside sync.cc. That would mean that the methods with no definitions will still be called.

comment:6 Changed 7 years ago by muks

BTW, note that without PTHREAD_MUTEX_ERRORCHECK, MutexTest.lockMultiple blocks indefinitely on Fedora (at the second lock attempt). I haven't tried behaviour on other platforms.

comment:7 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:8 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to muks

Hello

The most important comment I have is, the most expensive debug thing to be disabled is this:

// TODO: Distinguish if debug mode is enabled in compilation.
// If so, it should be PTHREAD_MUTEX_NORMAL or NULL
result = pthread_mutexattr_settype(&attributes, PTHREAD_MUTEX_ERRORCHECK);

About the locked multiple test, we probably want to disable it completely. Because most of it is ifdefed-out anyway and the only part left is the one that doesn't work with normal (fast) mutexes.

The thing with config.h ‒ for one, it is not local header, it should definitely not be included with quotes, but angle brackets. For another, while sync.h is mostly private header, I think we should distribute it (for one, because when we split the build into multiple packages, we need to refer to installed version). So I think we should ifdef inside the .cc file and not provide the methods in non-debug build (so they would cause linker error). And add a note about it to the documentation.

Then, few minor things:

There is some test or something left in the auth server:

auth_srv.o: In function `AuthSrv::swapDataSrcClientLists(boost::shared_ptr<std::map<isc::dns::RRClass, boost::shared_ptr<isc::datasrc::ConfigurableClientList>, std::less<isc::dns::RRClass>, std::allocator<std::pair<isc::dns::RRClass const, boost::shared_ptr<isc::datasrc::ConfigurableClientList> > > > >)':
/home/vorner/work/bind10/bind10-devel-20120817/_build/src/bin/auth/../../../../src/bin/auth/auth_srv.cc:939: undefined reference to `isc::util::thread::Mutex::locked() const'
auth_srv.o: In function `AuthSrvImpl::getDataSrcClientList(isc::dns::RRClass const&)':
/home/vorner/work/bind10/bind10-devel-20120817/_build/src/bin/auth/../../../../src/bin/auth/auth_srv.cc:278: undefined reference to `isc::util::thread::Mutex::locked() const'
/home/vorner/work/bind10/bind10-devel-20120817/_build/src/bin/auth/../../../../src/bin/auth/auth_srv.cc:278: undefined reference to `isc::util::thread::Mutex::locked() const'
collect2: ld returned 1 exit status
make[7]: *** [b10-auth] Error 1
make[7]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20120817/_build/src/bin/auth'

This comment may be removed or revised with the non-debug mode already existing.

// This assertion would fail only in non-debugging mode, in which case
// this method wouldn't be called either, so we simply assert the
// condition.

The „enable debugging“ in configure, I think it should be made clear this is
just some expensive debugging, so that people don't fear they'd get no
debugging support at all when not enabling it and they don't enable it for
production code, because it would be slow.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to vorner

Hi Michal

Replying to vorner:

Hello

The most important comment I have is, the most expensive debug thing to be disabled is this:

// TODO: Distinguish if debug mode is enabled in compilation.
// If so, it should be PTHREAD_MUTEX_NORMAL or NULL
result = pthread_mutexattr_settype(&attributes, PTHREAD_MUTEX_ERRORCHECK);

About the locked multiple test, we probably want to disable it completely. Because most of it is ifdefed-out anyway and the only part left is the one that doesn't work with normal (fast) mutexes.

Updated. :)

The thing with config.h ‒ for one, it is not local header, it should definitely not be included with quotes, but angle brackets.

We have to decide upon an include style that we can use in the entire tree, and add it to our CodingGuidelines wiki page. I'm following some convention when I write such includes.

In my understanding, the choice of include style has nothing to do with whether the header is local or not.

  • <> replaces what's inside the delimiters by something that is implementation dependent.. it could be a file, or something else
  • "" replaces what's inside the delimiters by a file (where it is found is implementation dependent.. usually -I, system directories, etc.)

I spoke to Jinmei about it, and he said that we can defer it to when we do a cleanup of the entire tree based on what style we choose.

For another, while sync.h is mostly private header, I think we should distribute it (for one, because when we split the build into multiple packages, we need to refer to installed version). So I think we should ifdef inside the .cc file and not provide the methods in non-debug build (so they would cause linker error). And add a note about it to the documentation.

I've moved the config.h to the .cc file now.

Then, few minor things:

There is some test or something left in the auth server:

auth_srv.o: In function `AuthSrv::swapDataSrcClientLists(boost::shared_ptr<std::map<isc::dns::RRClass, boost::shared_ptr<isc::datasrc::ConfigurableClientList>, std::less<isc::dns::RRClass>, std::allocator<std::pair<isc::dns::RRClass const, boost::shared_ptr<isc::datasrc::ConfigurableClientList> > > > >)':
/home/vorner/work/bind10/bind10-devel-20120817/_build/src/bin/auth/../../../../src/bin/auth/auth_srv.cc:939: undefined reference to `isc::util::thread::Mutex::locked() const'
auth_srv.o: In function `AuthSrvImpl::getDataSrcClientList(isc::dns::RRClass const&)':
/home/vorner/work/bind10/bind10-devel-20120817/_build/src/bin/auth/../../../../src/bin/auth/auth_srv.cc:278: undefined reference to `isc::util::thread::Mutex::locked() const'
/home/vorner/work/bind10/bind10-devel-20120817/_build/src/bin/auth/../../../../src/bin/auth/auth_srv.cc:278: undefined reference to `isc::util::thread::Mutex::locked() const'
collect2: ld returned 1 exit status
make[7]: *** [b10-auth] Error 1
make[7]: Leaving directory `/home/vorner/work/bind10/bind10-devel-20120817/_build/src/bin/auth'

Ouch.. fixed.

This comment may be removed or revised with the non-debug mode already existing.

// This assertion would fail only in non-debugging mode, in which case
// this method wouldn't be called either, so we simply assert the
// condition.

Removed.

The „enable debugging“ in configure, I think it should be made clear this is
just some expensive debugging, so that people don't fear they'd get no
debugging support at all when not enabling it and they don't enable it for
production code, because it would be slow.

configure.ac has been updated for it.

comment:10 Changed 7 years ago by muks

s/replaces what's inside the delimiters/replaces the directive based on what's inside the delimiters/

comment:11 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to muks

Hello

Replying to muks:

In my understanding, the choice of include style has nothing to do with whether the header is local or not.

  • <> replaces what's inside the delimiters by something that is implementation dependent.. it could be a file, or something else
  • "" replaces what's inside the delimiters by a file (where it is found is implementation dependent.. usually -I, system directories, etc.)

Hmm, what I thought was that both replace by a file and both are implementation
dependant. But in practice, "" first searches the local directory, then -I
paths, while <> does the -I paths only. At least from how I saw them used
across projects, if something is included by "", I tend to expect that
file/path is to be located relatively from the file it is included into.

I spoke to Jinmei about it, and he said that we can defer it to when we do a cleanup of the entire tree based on what style we choose.

OK.

This TODO is left in the code:

#ifdef ENABLE_DEBUG
    // TODO: Distinguish if debug mode is enabled in compilation.

Also, do you know the practical difference between PTHREAD_MUTEX_NORMAL and
PTHREAD_MUTEX_DEFAULT? It looks like the PTHREAD_MUTEX_DEFAULT leaves it up
to the compiler to decide if to use PTHREAD_MUTEX_NORMAL or
PTHREAD_MUTEX_ERRORCHECK. If that's your impression too, maybe it would be
better to explicitly ask for the PTHREAD_MUTEX_NORMAL?

Thanks

comment:12 in reply to: ↑ 11 Changed 7 years ago by muks

  • Owner changed from muks to vorner

Replying to vorner:

This TODO is left in the code:

#ifdef ENABLE_DEBUG
    // TODO: Distinguish if debug mode is enabled in compilation.

Removed. Another such TODO was also removed.

Also, do you know the practical difference between PTHREAD_MUTEX_NORMAL and
PTHREAD_MUTEX_DEFAULT? It looks like the PTHREAD_MUTEX_DEFAULT leaves it up
to the compiler to decide if to use PTHREAD_MUTEX_NORMAL or
PTHREAD_MUTEX_ERRORCHECK. If that's your impression too, maybe it would be
better to explicitly ask for the PTHREAD_MUTEX_NORMAL?

The attr types are described in the pthread_mutexattr_settype() POSIX documentation. I've updated the code to use PTHREAD_MUTEX_NORMAL by default.

comment:13 Changed 7 years ago by vorner

  • Owner changed from vorner to muks
  • Total Hours changed from 0 to 1.74

OK, thanks. It looks good now, please merge.

comment:14 Changed 7 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master in commit 653d1d854ad6574806ed09f6b42aaa4a17f27b57:

* 54ae641 [2236] Remove obsolete comment
* 1d4d9e7 [2236] Use PTHREAD_MUTEX_NORMAL fast mutexes by default
* 7c4e02c [2236] Remove obsolete comment
* 2eaf019 [2236] Don't use PTHREAD_MUTEX_ERRORCHECK in non-debug builds
* 8b19f3a [2236] Remove config.h include from sync.h
* 98db15a [2236] Add comments to configure.ac about --enable-debug and performance
* 4f4b372 [2236] Access Mutex::locked() only when ENABLE_DEBUG is defined
* fa78c4b [2236] Include and use the define from config.h
* 149df5a [2236] Add status whether debugging is enabled in configure output
* 2a6f076 [2236] Use ENABLE_DEBUG within the lock implementation
* 4887037 [2236] Add a --enable-debug configure flag

Resolving as fixed. Thank you for the reviews Michal.

Note: See TracTickets for help on using tickets.