Diverse memory handling reviews

[quique123]

Hi everyone, Chris,

As background info, I use XmlRpc++ in projects in embedded linux, using GCC/G++ compiler, in a HW enviroment with 64MB RAM (there should run linux + root filesystem + applications). That makes around 45 MB of "free RAM"

1) One use of the XML RPC is for transmitting files between a far end (a "PC") and the embedded device (a "device"), using a XmlRpc::BinaryValue. The device works as XmlRpc server, the PC reads the file binary information and send it to the device. In the device, XmlRpcServerConnection::parseRequest is called when the message inconming, and from there, the function:
bool XmlRpcValue::binaryFromXml(std::string const& valueXml, int* offset)

is called. There, you can see:
// ...
std::string asString = valueXml.substr(*offset, valueEnd-*offset);
// ...

decoder.get(asString.begin(), asString.end(), ins, iostatus);
// ..

That means, if a 10MB file is transmitted, with the substr () is duplicated the info (the header overhead is low) and then, the decoder.get () extracts the information and copy it in a buffer, that are additional 12MB.

That means, for this procedure 10+10+12 = 32 MB are needed.

In fact, the substr() call is not needed:

  // Base64
  bool XmlRpcValue::binaryFromXml(std::string const& valueXml, int* offset)
  {
    size_t valueEnd = valueXml.find('<', *offset);
    if (valueEnd == std::string::npos)
      return false;     // No end tag;

    // Value type setting and dinamically allocated memory
    _type = TypeBase64;
    _value.asBinary = new BinaryData();

    // convert from base64 to binary
    int iostatus = 0;
    base64<char> decoder;
   
    // Data needed for the convertion:
    // start iterator: beginning of the xml value
    // end iterator: position of the '<' character
    // where to insert: iterator in the new BinaryData object
    std::string::const_iterator startIt = valueXml.begin ();
    for (int i=0;i<*offset;i++)
        startIt++;
    std::string::const_iterator endIt = valueXml.end ();
    for (unsigned int i=valueEnd; i<(valueXml.length() - (valueEnd+*offset)); i++)
        endIt--;
    std::back_insert_iterator<BinaryData> ins = std::back_inserter(*(_value.asBinary));
   
    // Convertion
    decoder.get(startIt, endIt, ins, iostatus);
    *offset += valueEnd;
   
    return true;
  }

2) The second problem I got with the library is the implementation of std::string in the std++ library we use (5.0.5). Once allocated, the memory cannot be so easyly deallocated. According to the standards, neither clear nor erase, resize, remove, ... deallocate the memory. That makes, if a client or a connection object receives/sends a big string (i.e. for the file transmission I previously mentioned), the memory remains allocated as long as the connection remains open.

What I changed is the definition of the object data from string objects to pointer, and after its usage, if the size is bigger than 10K, then free the memory and reallocate it again.
i.e. in:
class XmlRpcServerConnection : public XmlRpcSource {
// ...
    std::string *_request; // Before, it was just std::string _request;
// ...

Then:      
std::string
XmlRpcServerConnection::parseRequest(XmlRpcValue& params)
{
  // ...
  // Free the request string memory, after parsing it, if needed
  if (_request->length () > 10*1024)
  {
    delete _request;
    _request = new std::string ("");
  }
 
  return methodName;
}

Same with XmlRpcClient data:
    std::string *_request;
    std::string *_response;

What do you thing of this approach?

If needed, I can provide a complete diff file for files XmlRpcClient.* and XmlRpcServerConnection.*

Regards,

José

[quique123]

The above code is buggy and contains some errors in the function
XmlRpcValue::binaryFromXml

Use the following:
// Base64
  bool XmlRpcValue::binaryFromXml(std::string const& valueXml, int* offset)
  {
    size_t valueEnd = valueXml.find('<', *offset);
    if (valueEnd == std::string::npos)
      return false;     // No end tag;

    // Value type setting
    _type = TypeBase64;
   
    // Instead of using a value, two iterators are used:
    // con
    _value.asBinary = new BinaryData();
    // check whether base64 encodings can contain chars xml encodes...

    // convert from base64 to binary
    int iostatus = 0;
    base64<char> decoder;
   
    // Data needed for the convertion:
    // start iterator: beginning of the xml value
    // end iterator: position of the '<' character
    // where to insert: iterator in the new BinaryData object
    std::string::const_iterator startIt = valueXml.begin ();
    std::string::const_iterator endIt = valueXml.begin ();
    unsigned int index = 0;
    for (index=0;(int) index<*offset;index++)
    {
        startIt++;
        endIt++;
    }
    for (;index<valueEnd;index++)
        endIt++;
    std::back_insert_iterator<BinaryData> ins = std::back_inserter(*(_value.asBinary));
   
    // Convertion
    decoder.get(startIt, endIt, ins, iostatus);

    *offset = valueEnd;
    return true;
  }

[Chris Morley]

How about just adding the values to the iterators directly rather than iteratively:
...
std::string::const_iterator startIt = valueXml.begin() + *valueOffset;
std::string::const_iterator endIt = valueXml.begin() + valueEnd;
...

Regarding your second point in the first message, the typical idiom for releasing memory from std::strings and std::vectors is to use swap with a temporary object:

_request.swap(std::string());

The empty data of the temporary object is swapped with the _request string buffer, then the existing buffer is deleted along with the temporary variable, with no explicit calls to new/delete needed.

[quique123]

Regarding the iterators, it is a good idea adding the valus.

But I try to use the function swap () and the compiler did not like it:

src/XmlRpcServerConnection.cpp: In member function `bool
   XmlRpc::XmlRpcServerConnection::readHeader()':
src/XmlRpcServerConnection.cpp:154: error: no matching function for call to `
   std::basic_string<char, std::char_traits<char>, std::allocator<char> >::swap
   (std::string)'
/opt/eldk/ppc_6xx/usr/include/c++/3.3.3/bits/basic_string.tcc:469: error: candidates
   are: void std::basic_string<_CharT, _Traits,
   _Alloc>::swap(std::basic_string<_CharT, _Traits, _Alloc>&) [with _CharT =
   char, _Traits = std::char_traits<char>, _Alloc = std::allocator<char>]

I think a possible solution is to use a temporary string, limited the scope by brackets, but it will make the memory be double during the defined scope. And even if the swap had work, the problem would have been the same: during the swap execution the memory needed by _request is doubled in the temporary object, and then deleted.

Therefore I still think the new/delete option is better for systems with strong memory limitations.

[Chris Morley]

As you say, maybe the compiler would like an explicit temporary.

VC 6 is broken in many ways. Given that vc++ 2005 and 2008 are free to download and use, maintaining vc 6 compatibility doesn't seem very important.

But you are not understanding swap if you think it will double the memory use. It does not copy the memory, it simply exchanges the pointer values of the internal char* buffers.

[quique123]

Regarding the memory, I had a look into the swap function and you are right.

Compiler: I use GCC 3.3.3, with PowerPC as plattform. I eliminated all initializations with = "" format and made an init string function with explicit empty string defined. It works.

Thanks, Chris

[Dragonyte]

Hey there,

on account of your first issue, I totally agree with you, and the problem is not just memory but also time (I'm moving around 15M pixel images in binary form).

But let me suggest a cleaner code for this method with no loops :

// Base64
  bool XmlRpcValue::binaryFromXml(std::string const& valueXml, int* offset)
  {
    size_t valueEnd = valueXml.find('<', *offset);
    if (valueEnd == std::string::npos)
      return false;     // No end tag;
    _type = TypeBase64;
    _value.asBinary = new BinaryData();
    // check whether base64 encodings can contain chars xml encodes...
    // convert from base64 to binary
    int iostatus = 0;
      base64<char> decoder;
    std::back_insert_iterator<BinaryData> ins = std::back_inserter(*(_value.asBinary));
            decoder.get(&valueXml[*offset], &valueXml[valueEnd], ins, iostatus);
    *offset = valueEnd;
    return true;
  }

I'm also getting into the base64.h methods now to see where we can save memory and time…

Best,
Ofir.