[esnacc-dev] [PATCH] cxx-lib: do not use exceptions in performance critical operators

[Aaron Conole]

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;