Optimisation challenge...
-
Hi all, I've got a nasty looking chuck of code that is just asking to be written better... The function as it is works... It converts the two double parameters to string format (using a macro that I havent added below), and then joins them all together (along with the headers that are already in string format). Would be great if someone could let me know a more eligant way to join the strings than the way I have done it...
static inline STRING lcd_ConstructLine(STRING pcH1, double dV1, STRING pcH2, double dV2) { char acBuff1[CONV_DOUBLE_STRING_SIZE]; char acBuff2[CONV_DOUBLE_STRING_SIZE]; strcpy(acBuff1, lcd_String(dV1)); strcpy(acBuff2, lcd_String(dV2)); strcpy(_acLine, pcH1); strcat(_acLine, acBuff1); strcat(_acLine, pcH2); strcat(_acLine, acBuff2); return(_acLine); }Mark Brock "We're definitely not going to make a G or a PG version of this. It's not PillowfightCraft." -- Chris Metzen
-
Hi all, I've got a nasty looking chuck of code that is just asking to be written better... The function as it is works... It converts the two double parameters to string format (using a macro that I havent added below), and then joins them all together (along with the headers that are already in string format). Would be great if someone could let me know a more eligant way to join the strings than the way I have done it...
static inline STRING lcd_ConstructLine(STRING pcH1, double dV1, STRING pcH2, double dV2) { char acBuff1[CONV_DOUBLE_STRING_SIZE]; char acBuff2[CONV_DOUBLE_STRING_SIZE]; strcpy(acBuff1, lcd_String(dV1)); strcpy(acBuff2, lcd_String(dV2)); strcpy(_acLine, pcH1); strcat(_acLine, acBuff1); strcat(_acLine, pcH2); strcat(_acLine, acBuff2); return(_acLine); }Mark Brock "We're definitely not going to make a G or a PG version of this. It's not PillowfightCraft." -- Chris Metzen
You're never going to get anything that elegant all the time you're using macros, global variables and pointers. As it looks like you're using a C++ compiler why not actually use the language?
std::string lcd_construct_line( const std::string &header_1, const std::string &header_2, double value_1, double value_2 )
{
std::ostringstream str;
str << header_1 << lcd_String( value_1 ) << header_2 << lcd_String( value_2 );
return str.str;
}I'd also get rid of the macro and, if it was used enough, implement a manipulator that does the same thing OR if it's only used once bung it in a function:
std::string lcd_construct_line( const std::string &header_1, const std::string &header_2, double value_1, double value_2 )
{
std::ostringstream str;
str << header_1 << lcd_string << value_1 << header_2 << lcd_string << value_2;
return str.str;
}It's not going to be much slower than the low level fiddling you're doing as the compiler can inline the insertions fairly effectively. Cheers, Ash
-
You're never going to get anything that elegant all the time you're using macros, global variables and pointers. As it looks like you're using a C++ compiler why not actually use the language?
std::string lcd_construct_line( const std::string &header_1, const std::string &header_2, double value_1, double value_2 )
{
std::ostringstream str;
str << header_1 << lcd_String( value_1 ) << header_2 << lcd_String( value_2 );
return str.str;
}I'd also get rid of the macro and, if it was used enough, implement a manipulator that does the same thing OR if it's only used once bung it in a function:
std::string lcd_construct_line( const std::string &header_1, const std::string &header_2, double value_1, double value_2 )
{
std::ostringstream str;
str << header_1 << lcd_string << value_1 << header_2 << lcd_string << value_2;
return str.str;
}It's not going to be much slower than the low level fiddling you're doing as the compiler can inline the insertions fairly effectively. Cheers, Ash
-
Hi all, I've got a nasty looking chuck of code that is just asking to be written better... The function as it is works... It converts the two double parameters to string format (using a macro that I havent added below), and then joins them all together (along with the headers that are already in string format). Would be great if someone could let me know a more eligant way to join the strings than the way I have done it...
static inline STRING lcd_ConstructLine(STRING pcH1, double dV1, STRING pcH2, double dV2) { char acBuff1[CONV_DOUBLE_STRING_SIZE]; char acBuff2[CONV_DOUBLE_STRING_SIZE]; strcpy(acBuff1, lcd_String(dV1)); strcpy(acBuff2, lcd_String(dV2)); strcpy(_acLine, pcH1); strcat(_acLine, acBuff1); strcat(_acLine, pcH2); strcat(_acLine, acBuff2); return(_acLine); }Mark Brock "We're definitely not going to make a G or a PG version of this. It's not PillowfightCraft." -- Chris Metzen
-
a lot of high level operators, than you and add MFC strings :rolleyes:
Press F1 for help or google it. Greetings from Germany
-
Hi all, I've got a nasty looking chuck of code that is just asking to be written better... The function as it is works... It converts the two double parameters to string format (using a macro that I havent added below), and then joins them all together (along with the headers that are already in string format). Would be great if someone could let me know a more eligant way to join the strings than the way I have done it...
static inline STRING lcd_ConstructLine(STRING pcH1, double dV1, STRING pcH2, double dV2) { char acBuff1[CONV_DOUBLE_STRING_SIZE]; char acBuff2[CONV_DOUBLE_STRING_SIZE]; strcpy(acBuff1, lcd_String(dV1)); strcpy(acBuff2, lcd_String(dV2)); strcpy(_acLine, pcH1); strcat(_acLine, acBuff1); strcat(_acLine, pcH2); strcat(_acLine, acBuff2); return(_acLine); }Mark Brock "We're definitely not going to make a G or a PG version of this. It's not PillowfightCraft." -- Chris Metzen
you could probably _snprintf all the parts into the output string. alternately, if you want to improve performance, take a look at all those strcat calls. each one of those does a scan of the output string, looking for the end.
strcpy(_acLine, pcH1);
strcat(_acLine, acBuff1); <-- full pass over _acLine
strcat(_acLine, pcH2); <-- another full pass over _acLine
strcat(_acLine, acBuff2); <-- another full pass over _acLinebut if you know the lengths of pcH1, acBuff1, pch2 and acBuff2, you will know where the end of _acLine should be after each concatenation. so you can simply memcpy the substrings directly where they should be:
memcpy(_acLine, pcH1, len_pcH1);
memcpy(_acLine + len_pcH1, acBuff1, len_buff1 );
memcpy(_acLine + len_pcH1 + len_buff1, pcH2, len_pcH2 );
memcpy(_acLine + len_pcH1 + len_buff1 + len_pcH2, acBuff2, len_buff2 );
acLine[len_pcH1 + len_buff1 + len_pcH2 + buff2] = 0;and if you know the length of acBuff1 and acBuff2 then maybe you could do this:
memcpy(_acLine, pcH1, len_pcH1);
memcpy(_acLine + len_pcH1, lcd_String(dV1), len_buff1 );
memcpy(_acLine + len_pcH1 + len_buff1, pcH2, len_pcH2 );
memcpy(_acLine + len_pcH1 + len_buff1 + len_pcH2, lcd_String(dV2), len_buff2 );
acLine[len_pcH1 + len_buff1 + len_pcH2 + buff2] = 0;... and get rid of those first two strcpys, too. or, if you want to get really low-level, just concat the strings yourself. something like:
LPSTR pOut = _acLine;
while (*pcH1) *pOut++=*pcH1++;
while (*acBuff1) *pOut++=*acBuff1++;
while (*pcH2) *pOut++=*pcH2++;
while (*acBuff2) *pOut++=*acBuff2++;
*pOut = 0;image processing toolkits | batch image processing
modified on Wednesday, June 30, 2010 10:25 AM
-
MFC strings are a complete aberration - about all they're good for is the last thing you convert to before throwing a string at an MFC function. Just say no to 1992 implementations of strings! Ash
MS has done a fine job since than and rewritten the code every MFC-Release. to be clear: the MFC-String is for that fully ovewerbloated, but handy for the coder. And sometimes THIS is the key reason for using.
Press F1 for help or google it. Greetings from Germany
-
You're never going to get anything that elegant all the time you're using macros, global variables and pointers. As it looks like you're using a C++ compiler why not actually use the language?
std::string lcd_construct_line( const std::string &header_1, const std::string &header_2, double value_1, double value_2 )
{
std::ostringstream str;
str << header_1 << lcd_String( value_1 ) << header_2 << lcd_String( value_2 );
return str.str;
}I'd also get rid of the macro and, if it was used enough, implement a manipulator that does the same thing OR if it's only used once bung it in a function:
std::string lcd_construct_line( const std::string &header_1, const std::string &header_2, double value_1, double value_2 )
{
std::ostringstream str;
str << header_1 << lcd_string << value_1 << header_2 << lcd_string << value_2;
return str.str;
}It's not going to be much slower than the low level fiddling you're doing as the compiler can inline the insertions fairly effectively. Cheers, Ash
Hi Aescleal, Thanks for your answer. I should been more explicit though. Its a C compiler, STRING is a type definition. The code is in an Atmel microcontroller and has very little space left in flash for more code, that was why I used the global variable instead of a local pointer each time the function was called. A work mate pointed out I could use sprintf.. which would likely be no faster, but it would be much cleaner looking. Your right about the macro calls - I'm going to take those out. Thanks,
Mark Brock "We're definitely not going to make a G or a PG version of this. It's not PillowfightCraft." -- Chris Metzen
-
you could probably _snprintf all the parts into the output string. alternately, if you want to improve performance, take a look at all those strcat calls. each one of those does a scan of the output string, looking for the end.
strcpy(_acLine, pcH1);
strcat(_acLine, acBuff1); <-- full pass over _acLine
strcat(_acLine, pcH2); <-- another full pass over _acLine
strcat(_acLine, acBuff2); <-- another full pass over _acLinebut if you know the lengths of pcH1, acBuff1, pch2 and acBuff2, you will know where the end of _acLine should be after each concatenation. so you can simply memcpy the substrings directly where they should be:
memcpy(_acLine, pcH1, len_pcH1);
memcpy(_acLine + len_pcH1, acBuff1, len_buff1 );
memcpy(_acLine + len_pcH1 + len_buff1, pcH2, len_pcH2 );
memcpy(_acLine + len_pcH1 + len_buff1 + len_pcH2, acBuff2, len_buff2 );
acLine[len_pcH1 + len_buff1 + len_pcH2 + buff2] = 0;and if you know the length of acBuff1 and acBuff2 then maybe you could do this:
memcpy(_acLine, pcH1, len_pcH1);
memcpy(_acLine + len_pcH1, lcd_String(dV1), len_buff1 );
memcpy(_acLine + len_pcH1 + len_buff1, pcH2, len_pcH2 );
memcpy(_acLine + len_pcH1 + len_buff1 + len_pcH2, lcd_String(dV2), len_buff2 );
acLine[len_pcH1 + len_buff1 + len_pcH2 + buff2] = 0;... and get rid of those first two strcpys, too. or, if you want to get really low-level, just concat the strings yourself. something like:
LPSTR pOut = _acLine;
while (*pcH1) *pOut++=*pcH1++;
while (*acBuff1) *pOut++=*acBuff1++;
while (*pcH2) *pOut++=*pcH2++;
while (*acBuff2) *pOut++=*acBuff2++;
*pOut = 0;image processing toolkits | batch image processing
modified on Wednesday, June 30, 2010 10:25 AM
Hi Chris, Awesome answer! I do know all the (expected) lengths. I'll give your idea a go tonight! The first two strcpy calls are horrible... But for some reason the function wouldn't work without them... maybe something to do with the Atmel compiler.. Thanks,
Mark Brock "We're definitely not going to make a G or a PG version of this. It's not PillowfightCraft." -- Chris Metzen
-
Hi Aescleal, Thanks for your answer. I should been more explicit though. Its a C compiler, STRING is a type definition. The code is in an Atmel microcontroller and has very little space left in flash for more code, that was why I used the global variable instead of a local pointer each time the function was called. A work mate pointed out I could use sprintf.. which would likely be no faster, but it would be much cleaner looking. Your right about the macro calls - I'm going to take those out. Thanks,
Mark Brock "We're definitely not going to make a G or a PG version of this. It's not PillowfightCraft." -- Chris Metzen
It was the inline that made me think it was a C++ compiler - I'm not up on C99 (which presumably added it) so sorry about any confusion. While sprintf is a bit cleaner one thing to watch out for is the size of the code you get linked in when you use sprintf. It's a bit of a monster function so if it's not already linked in the code base you might be better off using strcpy/memcpy [1] to save space. If you're in a corner try both versions and see which is smaller. Another slight problem with sprintf is that the function has got to parse the format string before it can actually copy anything which usually makes:
sprintf( buff, "%s%s" );
a pretty slow way of doing strcpy and strcat. One final point (and if you're maintaining a code base it's not something you can change easily to) is to use length prepended strings instead of zero terminated ones. As one of the other posters (Chris L IIRC) pointed out if you know the size of the strings you can use memcpy instead of strcpy. Prepending the size saves you the balls ache of calling strcpy everywhere at the cost of a bit more arithmetic to maintain the length. Cheers, Ash [1] Just about every compiler I've used since 1991 has inlined calls to strcpy and memcpy as it eliminates the function call overhead which is faster and saves space. I've never seen an inlined sprintf implementation in C though.
-
Hi Chris, Awesome answer! I do know all the (expected) lengths. I'll give your idea a go tonight! The first two strcpy calls are horrible... But for some reason the function wouldn't work without them... maybe something to do with the Atmel compiler.. Thanks,
Mark Brock "We're definitely not going to make a G or a PG version of this. It's not PillowfightCraft." -- Chris Metzen
be sure to try that last chunk of code. that should out-perform any of the others.
-
be sure to try that last chunk of code. that should out-perform any of the others.
Will do... I'm going to give this a crack too.. http://en.wikipedia.org/wiki/Duff%27s_device It will be interesting to see the different execution times of all these methods.
Mark Brock "We're definitely not going to make a G or a PG version of this. It's not PillowfightCraft." -- Chris Metzen