I was looking more closely at some performance issues and came up with a modification of how the resulting xml/wbxml is assembled. Instead of string concatenation, I am using a buffer class, which collects the strings. The buffer is extended automatically, if necessary. The problem with string concatenations are the numerous memory reallocations, that are necessary as the string grows.
With my solution, I was able to improve the performance (again measured with val1_server/client, persistent connections) by another 30%.
In absolute numbers:
Machine: 2 GHz Dual Core
server and client on same machine
val1_client 1000 iterations, persistent connections
original implementation: around 58 secs
alternative ulxr::Value + new storage model: 36 secs
The improvement is especially impressive for large data sets.
I used the test moderateSizeArrayCheck with an array size of 2.5 millions, which amounts to an xml file of around 130 MB. This test runs in 18 secs with the optimizations, whereas the original takes 10+ minutes.
This optimization is not completely worked out yet, especially with respect to Unicode support. It also requires changes to some classes in the framework. However, I'd be happy to contribute the code, if desired.
P.K.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I am not sure about the allocation problem. Basically the string class should take care about reasonable few reallocations. There may certainly be situations (130MB strings :-) where there is a lot of unnecessary allocating and especially moving. But I suspect there are some tricky parts involved as you need to find out the resulting size very early to preallocate all the memory. For a "normal" transmission there will probably be no measurable difference. But anyway, I would like to see your patches :-)
As for your previous post: I integrated your Value patches and had to apply a bunch of fixes due to unicode and the modified hierarchy below Value. And as far as I know, hash_map is obsolete so I conditionally replaced with map. And
And due to the fine weather I did no real tests yet :-)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> I am not sure about the allocation problem. Basically the string class
> should take care about reasonable few reallocations.
I think you are expecting too much from std::string. Actually, there is an alternative STL class std::rope, which especially has been created for constructing large strings, BECAUSE std::string has the reallocation problem (as you said, its not only the allocation itself, but moving the data every time). And this effect is noticable even if your resulting string is only some tens kB. I also think that the transmission of binary data is also not so uncommon, which might easily sum up to several MBs (I saw an older post discussing this issue). Certainly, std::string does anticipate concatenation and you can force it to do so by using reserve(), but I was looking for a better solution ;-)
> But I suspect there are some tricky parts involved as you need to find
> out the resulting size very early to preallocate all the memory.
Not at all, the only trick is not to store everything in a single continuous memory chunk, but in blocks. So, in my implementation, blocks with a fixed size are allocated on demand, as the data grows. I created a storage class, which has stream semantics. So instead of adding things to a string, they are streamed into the storage. But true, using it requires a couple of changes to some of the interfaces. And due to the internals of the storage class, several calls to HttpProtocl::writeBody, one for each memory block, are necessary; I don't know, if that poses a problem.
Then e.g. HttpProtocol::sendRpcResponse looks like this:
...
Store xml;
resp.getXml(xml,0);
xml << ULXR_PCHAR("\n");
sendResponseHeader(200, ULXR_PCHAR("OK"), ULXR_PCHAR("text/xml"), xml.length(), wbxml_mode);
xml.write(*this);
...
and
MethodResponse::getXml looks like this:
ULXR_API_IMPL(void) MethodResponse::getXml(Store &s,int indent) const
{
CppString ind = getXmlIndent(indent);
CppString ind1 = getXmlIndent(indent+1);
CppString ind2 = getXmlIndent(indent+2);
s << ULXR_PCHAR("<?xml version=\"1.0\" encoding=\"utf-8\"?>") << getXmlLinefeed();
s << ind << ULXR_PCHAR("<methodResponse>") << getXmlLinefeed();
if (wasOk)
{
s << ind1 << ULXR_PCHAR("<params>") << getXmlLinefeed();
if (!respval.isVoid())
{
s << ind2 << ULXR_PCHAR("<param>") << getXmlLinefeed();
respval.getXml(s,indent+3);
s << getXmlLinefeed();
...
I also have a suggestion for the unicode build. Running unicodeToUtf8 on the generated xml is not optimal in my eyes (it is also not really 'compatible' with my storage approach. I think, it would be much better to have getXml always produce a Cpp8BitString and do the transcoding, only where it is necessary, during the construction of the xml.
> And as far as I know, hash_map is obsolete so I conditionally replaced with map.
What do you mean by obsolete? map and hash_map are different things. map is sorted and uses binary search, whereas hash_map uses hashing, thus being more performant. hash_map has not been part of STL originally, but I think every implementation has it now. Correct me, if I'm wrong...
After all my posts, you must think I'm a performance junkee.... well, probably I am. But seriously, I'm just trying to contribute to the lib to be a good multi-purpose communication solution for my needs without bloating it. And the lib is a wonderful starting point, since it is self-contained, well-structured and well-documented. Congratulations. If, after all, you don't find it worthwhile integrating my changes, no problem, I will publish them as required by the license on my site...
> And due to the fine weather I did no real tests yet
Happy you :-)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
>What do you mean by obsolete? map and hash_map are different things.
I remember vaguely having read something about this in an article about TR1 or C++0x. g++ does not have <hash_map> but only <backward/hash_map>.
>After all my posts, you must think I'm a performance junkee....
fine :-) I must admit here that I did not too much performance testing. As I mentioned already most of (my) transmissions are small and these issues have little impact here. The post you mentioned had a different background (converting entities). But I do appreciate patches which improve performance if they are portable and fit with the lib. So please contribute everything you have :-)
Currently I am busy with other things (e.g. work around my house and taking advantage of the weather :-) so I don't know when to complete and test the remaining stuff.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
during my recent holidays I spent some time with your suggestions. Since my time is currently limited and probably there will be not much more time in the near future I simply post the summary here:
* I renamed and reworked your Store class. First I gave "operator new[]" it's correct counterpart "operator delete[]" (which may be no actual problem anywhere :-) But a real problem was when writing a chunk larger than the internally fixed buffer. The class allocates the correct mem size but later forgets to copy back and free the remainder. I also tried an additional fixed size static buffer but had no actual improvement with it.
* The new Value class works a bit differently now. The test file xmlfunc did not compile. Nor did it run as it seems not possible anymore to assign on Value-based object to another of the same kind. I did not fully track it to the cause but I assume there is some pointer used twice when copying the data.
greets
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
very nice to hear that you are picking up the suggestions, that's motivating :-)
Sorry for the bug :-(. I think there is only a minor change needed to the original code in the add function, to make it work. But I want to take a second look at it to be absolutely sure, before I post the fix.
I'll also look into the xmlfunc sample and see if the problems can be solved.
After all, if that's finally all in the library, I would be absolutely happy with it :-)
Cheers
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I have now checked the string builder class and created a patch that resolves the problem. Basically, in the original code an 'if' had to be replaced with 'while'. The string remainder is properly taken care of. An additional static buffer is not necessary.
If have checked the code 'manually' with a few test cases and it should be good now.
You can find the patch against the latest ulxr_stringbuilder.h here:
Ok. I also got the Value problem fixed. The way, 'Value's are used in xmlfunc caused a cyclic dependency. So, I have added 'copy on write' semantics, which avoids the problems.
As far as I can see, xmlfunc now runs fine.
The patch against the latest ulxr_value_new.h/.cpp is here:
I was looking more closely at some performance issues and came up with a modification of how the resulting xml/wbxml is assembled. Instead of string concatenation, I am using a buffer class, which collects the strings. The buffer is extended automatically, if necessary. The problem with string concatenations are the numerous memory reallocations, that are necessary as the string grows.
With my solution, I was able to improve the performance (again measured with val1_server/client, persistent connections) by another 30%.
In absolute numbers:
Machine: 2 GHz Dual Core
server and client on same machine
val1_client 1000 iterations, persistent connections
original implementation: around 58 secs
alternative ulxr::Value + new storage model: 36 secs
The improvement is especially impressive for large data sets.
I used the test moderateSizeArrayCheck with an array size of 2.5 millions, which amounts to an xml file of around 130 MB. This test runs in 18 secs with the optimizations, whereas the original takes 10+ minutes.
This optimization is not completely worked out yet, especially with respect to Unicode support. It also requires changes to some classes in the framework. However, I'd be happy to contribute the code, if desired.
P.K.
Hello,
I am not sure about the allocation problem. Basically the string class should take care about reasonable few reallocations. There may certainly be situations (130MB strings :-) where there is a lot of unnecessary allocating and especially moving. But I suspect there are some tricky parts involved as you need to find out the resulting size very early to preallocate all the memory. For a "normal" transmission there will probably be no measurable difference. But anyway, I would like to see your patches :-)
As for your previous post: I integrated your Value patches and had to apply a bunch of fixes due to unicode and the modified hierarchy below Value. And as far as I know, hash_map is obsolete so I conditionally replaced with map. And
And due to the fine weather I did no real tests yet :-)
> I am not sure about the allocation problem. Basically the string class
> should take care about reasonable few reallocations.
I think you are expecting too much from std::string. Actually, there is an alternative STL class std::rope, which especially has been created for constructing large strings, BECAUSE std::string has the reallocation problem (as you said, its not only the allocation itself, but moving the data every time). And this effect is noticable even if your resulting string is only some tens kB. I also think that the transmission of binary data is also not so uncommon, which might easily sum up to several MBs (I saw an older post discussing this issue). Certainly, std::string does anticipate concatenation and you can force it to do so by using reserve(), but I was looking for a better solution ;-)
> But I suspect there are some tricky parts involved as you need to find
> out the resulting size very early to preallocate all the memory.
Not at all, the only trick is not to store everything in a single continuous memory chunk, but in blocks. So, in my implementation, blocks with a fixed size are allocated on demand, as the data grows. I created a storage class, which has stream semantics. So instead of adding things to a string, they are streamed into the storage. But true, using it requires a couple of changes to some of the interfaces. And due to the internals of the storage class, several calls to HttpProtocl::writeBody, one for each memory block, are necessary; I don't know, if that poses a problem.
You can have a look at the ulxr::Store class here:
http://www.elementec.de/download/code/ulxr_store.zip
Then e.g. HttpProtocol::sendRpcResponse looks like this:
...
Store xml;
resp.getXml(xml,0);
xml << ULXR_PCHAR("\n");
sendResponseHeader(200, ULXR_PCHAR("OK"), ULXR_PCHAR("text/xml"), xml.length(), wbxml_mode);
xml.write(*this);
...
and
MethodResponse::getXml looks like this:
ULXR_API_IMPL(void) MethodResponse::getXml(Store &s,int indent) const
{
CppString ind = getXmlIndent(indent);
CppString ind1 = getXmlIndent(indent+1);
CppString ind2 = getXmlIndent(indent+2);
s << ULXR_PCHAR("<?xml version=\"1.0\" encoding=\"utf-8\"?>") << getXmlLinefeed();
s << ind << ULXR_PCHAR("<methodResponse>") << getXmlLinefeed();
if (wasOk)
{
s << ind1 << ULXR_PCHAR("<params>") << getXmlLinefeed();
if (!respval.isVoid())
{
s << ind2 << ULXR_PCHAR("<param>") << getXmlLinefeed();
respval.getXml(s,indent+3);
s << getXmlLinefeed();
...
I also have a suggestion for the unicode build. Running unicodeToUtf8 on the generated xml is not optimal in my eyes (it is also not really 'compatible' with my storage approach. I think, it would be much better to have getXml always produce a Cpp8BitString and do the transcoding, only where it is necessary, during the construction of the xml.
> And as far as I know, hash_map is obsolete so I conditionally replaced with map.
What do you mean by obsolete? map and hash_map are different things. map is sorted and uses binary search, whereas hash_map uses hashing, thus being more performant. hash_map has not been part of STL originally, but I think every implementation has it now. Correct me, if I'm wrong...
After all my posts, you must think I'm a performance junkee.... well, probably I am. But seriously, I'm just trying to contribute to the lib to be a good multi-purpose communication solution for my needs without bloating it. And the lib is a wonderful starting point, since it is self-contained, well-structured and well-documented. Congratulations. If, after all, you don't find it worthwhile integrating my changes, no problem, I will publish them as required by the license on my site...
> And due to the fine weather I did no real tests yet
Happy you :-)
Hello,
>What do you mean by obsolete? map and hash_map are different things.
I remember vaguely having read something about this in an article about TR1 or C++0x. g++ does not have <hash_map> but only <backward/hash_map>.
>After all my posts, you must think I'm a performance junkee....
fine :-) I must admit here that I did not too much performance testing. As I mentioned already most of (my) transmissions are small and these issues have little impact here. The post you mentioned had a different background (converting entities). But I do appreciate patches which improve performance if they are portable and fit with the lib. So please contribute everything you have :-)
Currently I am busy with other things (e.g. work around my house and taking advantage of the weather :-) so I don't know when to complete and test the remaining stuff.
Hello Peter,
during my recent holidays I spent some time with your suggestions. Since my time is currently limited and probably there will be not much more time in the near future I simply post the summary here:
* I renamed and reworked your Store class. First I gave "operator new[]" it's correct counterpart "operator delete[]" (which may be no actual problem anywhere :-) But a real problem was when writing a chunk larger than the internally fixed buffer. The class allocates the correct mem size but later forgets to copy back and free the remainder. I also tried an additional fixed size static buffer but had no actual improvement with it.
http://ulxmlrpcpp.svn.sourceforge.net/viewvc/ulxmlrpcpp/trunk/ulxmlrpcpp/ulxmlrpcpp/ulxr_stringbuilder.h?view=markup
* The new Value class works a bit differently now. The test file xmlfunc did not compile. Nor did it run as it seems not possible anymore to assign on Value-based object to another of the same kind. I did not fully track it to the cause but I assume there is some pointer used twice when copying the data.
greets
Hi,
very nice to hear that you are picking up the suggestions, that's motivating :-)
Sorry for the bug :-(. I think there is only a minor change needed to the original code in the add function, to make it work. But I want to take a second look at it to be absolutely sure, before I post the fix.
I'll also look into the xmlfunc sample and see if the problems can be solved.
After all, if that's finally all in the library, I would be absolutely happy with it :-)
Cheers
Hi,
I have now checked the string builder class and created a patch that resolves the problem. Basically, in the original code an 'if' had to be replaced with 'while'. The string remainder is properly taken care of. An additional static buffer is not necessary.
If have checked the code 'manually' with a few test cases and it should be good now.
You can find the patch against the latest ulxr_stringbuilder.h here:
http://www.elementec.de/download/code/stringbuilder.patch
Next, I'll look at the value thing.
P.
Ok. I also got the Value problem fixed. The way, 'Value's are used in xmlfunc caused a cyclic dependency. So, I have added 'copy on write' semantics, which avoids the problems.
As far as I can see, xmlfunc now runs fine.
The patch against the latest ulxr_value_new.h/.cpp is here:
http://www.elementec.de/download/code/ulxr_value_new.patch
P.