[esnacc-dev] [PATCH] cxx-lib: do not use exceptions in performance critical operators
Aaron Conole
aconole at bytheb.org
Tue Mar 21 17:46:29 UTC 2017
Hi László,
Thanks very much for the contribution. It seems to be a bit corrupted,
so I'm going to ask if you could try sending as an attachment and see
if that works better.
Kovács László <laszlo.kovacs at neti.com> writes:
> In asn-buf.cpp the operator<() and the operator==() methods
> suffer from a performance issue, they are running in a loop until
> an exception is not thrown. This is a very bad idea, and very
> inefficient way of error handling. I have done some performance
> tests with gproof. The performance has improved about 4 times better,
> the code runs faster after my fix. I removed the exception handling from
> operator<() and the operator==() and replaced them to a
> boolean function, that can signal if an exception would be thrown.
>
> Signed-off-by: László Kovács <laszlo.kovacs at neti.com> <sabbath24 at gmail.com>
Which email address would you like for this contribution? Unless there
are two authors?
> Acked-by: László Kovács <laszlo.kovacs at neti.com> <sabbath24 at gmail.com>
Typically this tag won't be used by a patch submitter.
> Tested-by: László Kovács <laszlo.kovacs at neti.com> <sabbath24 at gmail.com>
> Reported-by: László Kovács <laszlo.kovacs at neti.com> <sabbath24 at gmail.com>
I would trim these tags as well. Reported-by: would be used to flag a
bug in a specific commit. This seems to be a general performance
improvement. Likewise, Tested-by is usually to flag that another person
submitted.
> ---
>
> diff --git a/esnacc-ng-master/cxx-lib/inc/asn-buf.h
> b/esnacc-ng-master/cxx-lib/inc/asn-buf.h
> index 4d06590..4235fb9 100644
> --- a/esnacc-ng-master/cxx-lib/inc/asn-buf.h
> +++ b/esnacc-ng-master/cxx-lib/inc/asn-buf.h
> @@ -32,13 +32,13 @@
> #define SNACCDLL_API __declspec(dllexport)
> #else
> #ifdef SNACCDLL_NONE
> -#define SNACCDLL_API
> +#define SNACCDLL_API
> #else
> #define SNACCDLL_API __declspec(dllimport)
> #endif
> #endif
> #else
> -#define SNACCDLL_API
> +#define SNACCDLL_API
> #endif
> #endif
This hunk would be better split into a separate patch for just
whitespace cleanup. That applies to most of the changes here, so I'm
dropping those hunks from my response.
> @@ -115,7 +115,9 @@ public:
> void skip(size_t skipBytes);
> char PeekByte() const;
> char GetByte() const;
> + bool GetByteNoEx(char& ret) const;
> unsigned char GetUByte() const { return (unsigned char)GetByte();}
> + bool GetUByteNoEx(unsigned char &ret) const { char val; bool
> tmp=GetByteNoEx(val); ret=(unsigned char)val; return tmp; }
I'm assuming this was meant to be on all one line, but wanted to
confirm.
If so, please format:
bool GetUByteNoEx(unsigned char &ret) const
{
char val;
bool tmp;
tmp = GetByteNoEx(val);
ret = (unsigned char)val;
return tmp;
}
or even better, something like:
bool GetUByteNoEx(unsigned char &ret) const
{
return GetByteNoEx((char &)ret);
}
...
> @@ -263,6 +263,27 @@ void AsnBuf::GetSeg(std::string &seg, long segLen) const
> }
> }
>
> +bool AsnBuf::GetByteNoEx(char& ret) const
> +{
> + std::streambuf::int_type ch;
> +
> + if(m_card==m_deck.end()) return false;
> +
> + do
> + {
> + ch=(*m_card)->rdbuf()->sbumpc();
> + if(ch==EOF)
> + {
> + ++m_card;
> + if(m_card==m_deck.end()) return false;
> + }
> + }
> + while(ch==EOF);
> +
> + ret=(char)ch;
> + return true;
> +}
> +
Would this work as well:
bool AsnBuf::GetByteNoEx(char &ret) const
{
std::streambuf::int_type ch = EOF;
while (ch == EOF && m_card != m_deck.end()) {
ch = (*m_card)->rdbuf()->sbumpc();
if (ch == EOF) {
++m_card;
}
}
ret = (char)ch;
return ch != EOF && m_card != m_deck.end();
}
> bool AsnBuf::operator<(const AsnBuf &rhs) const
> {
> bool lessThan = true;
> @@ -271,32 +292,23 @@ bool AsnBuf::operator<(const AsnBuf &rhs) const
> rhs.ResetMode();
> std::streambuf::int_type ch1;
> std::streambuf::int_type ch2;
> + unsigned char Ch1;
> + unsigned char Ch2;
> +
> while ( lessThan )
> {
> - try
> - {
> - ch1 = GetUByte();
> - }
> - catch (BufferException &)
> - {
> - ch1 = EOF;
> - }
> + if(GetUByteNoEx(Ch1)) ch1=Ch1;
> + else ch1=EOF;
For if conditions with else branches, please make it look like:
if (condition)
line
else
line
and obviously, if one side is multi-line, all sides should also be
multi-line.
> - try
> - {
> - ch2 = rhs.GetUByte();
> - }
> - catch (BufferException &)
> - {
> - ch2 = EOF;
> - }
> + if(rhs.GetUByteNoEx(Ch2)) ch2=Ch2;
> + else ch2=EOF;
Same comment applies.
> if ((ch1 == EOF) && (ch2 == EOF))
> {
> if (firstTime)
> lessThan = false;
> break;
> - }
> + }
> else if (ch2 == EOF)
> {
> lessThan = false;
> @@ -313,7 +325,9 @@ bool AsnBuf::operator<(const AsnBuf &rhs) const
> break;
>
> firstTime = false;
> +
Cut this ^^ line from your whitespace changes, though.
> }
> +
> ResetMode();
> rhs.ResetMode();
> return lessThan;
More information about the dev
mailing list