This minor clean-up provides a negligible performance improvement
to OBAtom::GetValence(), but addresses a commonly occurring but
poor C++/STL programming idiom in the OpenBabel source tree.
The inefficient idiom that's currently used looks like:
return _vbond.empty() ? 0 : _vbond.size();
Whilst it is true for many STL data structures that testing
empty() is often more efficient than testing size() == 0, it is
rarely beneficial to use empty() purely to avoid calling size()
as a special case. This is certainly the case with std::vector
implementations on modern hardware.
To demonstrate this, consider the following test code
#include <vector>
std::vector<void*> v;
unsigned int oldway() {
return v.empty() ? 0 : static_cast<unsigned int>(v.size());
}
unsigned int newway() {
return (unsigned int)v.size();
}
Using g++ v4.6.0 on x86_64, the original implementation requires
10 (9) instructions including a conditional jump
oldway: movq v+8(%rip), %rcx
movq v(%rip), %rdx
xorl %eax, %eax
cmpq %rdx, %rcx
je done
movq %rcx, %rax
subq %rdx, %rax
shrq $3, %rax
done: rep ; ret
whereas the obvious simpler idiom requires only 4 instructions
newway: movq v+8(%rip), %rax
subq v(%rip), %rax
sarq $3, %rax
ret
It turns out that this idiom/misunderstanding occurs in various places
throughout the source tree. In the attached patch I've fixed
GetValence()
[as I was investigating a valence-related SMARTS bug] and rotor.h as
another
occurrence in the same directory. However many examples still remain in
the main src/ directories including
typer.cpp: if (vs.empty() || vs.size() < 3)
rotor.cpp: if (!vs.empty() && vs.size() > 5)
phmodel.cpp: if (vs.empty() || vs.size() < 5)
mol.cpp: if (_vatom.empty() || _natoms+1 >=
(signed)_vatom.size())
and so on. There was once a time I'd have fixed this is g++, but I'm
getting old. Hopefully, there's a willing volunteer to audit and fix
the
source tree.
The following has been tested with "make test" with no regressions.
Index: include/openbabel/atom.h
===================================================================
*** include/openbabel/atom.h (revision 4680)
--- include/openbabel/atom.h (working copy)
*************** namespace OpenBabel
*** 227,236 ****
//! \deprecated Use GetCoordinateIdx() instead
unsigned int GetCIdx() const { return((int)_cidx); }
//! \return The current number of explicit connections
! unsigned int GetValence() const
! {
! return((_vbond.empty()) ? 0 : static_cast<unsigned int>
(_vbond.size()));
! }
//! \return The hybridization of this atom: 1 for sp, 2 for sp2, 3 for
sp3, 4 for sq. planar, 5 for trig. bipy, 6 for octahedral
unsigned int GetHyb() const;
//! \return The implicit valence of this atom type (i.e. maximum number
of connections expected)
--- 227,233 ----
//! \deprecated Use GetCoordinateIdx() instead
unsigned int GetCIdx() const { return((int)_cidx); }
//! \return The current number of explicit connections
! unsigned int GetValence() const { return (unsigned int)_vbond.size(); }
//! \return The hybridization of this atom: 1 for sp, 2 for sp2, 3 for
sp3, 4 for sq. planar, 5 for trig. bipy, 6 for octahedral
unsigned int GetHyb() const;
//! \return The implicit valence of this atom type (i.e. maximum number
of connections expected)
Index: include/openbabel/rotor.h
===================================================================
*** include/openbabel/rotor.h (revision 4680)
--- include/openbabel/rotor.h (working copy)
*************** namespace OpenBabel
*** 467,473 ****
*/
size_t Size()
{
! return((_rotor.empty()) ? (size_t)0: _rotor.size());
}
/**
* When no atoms/bonds are fixed or when bonds are fixed, this function
will
--- 467,473 ----
*/
size_t Size()
{
! return _rotor.size();
}
/**
* When no atoms/bonds are fixed or when bonds are fixed, this function
will
Thanks in advance,
Roger
--
Roger Sayle, Ph.D.
CEO and founder
NextMove Software Ltd
Cambridge, UK
------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of
discussion for anyone considering optimizing the pricing and packaging model
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/
_______________________________________________
OpenBabel-Devel mailing list
OpenBabel-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
