It seemed like a good idea.
commit beeba499793328cc118992884d8e175caffa6433 Author: Stephen R. van den Berg srb@cuci.nl Date: Thu Aug 21 13:17:21 2008 +0200
Optimise optimisation and move inline int2string from roxen.c to operators.c
diff --git a/src/modules/_Roxen/roxen.c b/src/modules/_Roxen/roxen.c index 253f2b3..dcf6086 100644 --- a/src/modules/_Roxen/roxen.c +++ b/src/modules/_Roxen/roxen.c @@ -456,33 +456,6 @@ static void f_html_encode_string( INT32 args ) void o_cast_to_string();
case PIKE_T_INT: - /* Optimization, this is basically a inlined cast_int_to_string */ - { - char buf[21], *b = buf+19; - int neg, i, j=0; - i = Pike_sp[-1].u.integer; - pop_stack(); - if( i < 0 ) - { - neg = 1; - i = -i; - } - else - neg = 0; - - buf[20] = 0; - - while( i >= 10 ) - { - b[ -j++ ] = '0'+(i%10); - i /= 10; - } - b[ -j++ ] = '0'+(i%10); - if( neg ) b[ -j++ ] = '-'; - push_text( b-j+1 ); - } - return; - case PIKE_T_FLOAT: /* Optimization, no need to check the resultstring for * unsafe characters. diff --git a/src/operators.c b/src/operators.c index 3d12cbd..d1eaf75 100644 --- a/src/operators.c +++ b/src/operators.c @@ -343,11 +343,10 @@ PMOD_EXPORT void o_cast_to_int(void) /* Special case for casting to string. */ PMOD_EXPORT void o_cast_to_string(void) { - char buf[200]; switch(sp[-1].type) { case PIKE_T_STRING: - return; + break;
case T_OBJECT: if(!sp[-1].u.object->prog) { @@ -387,10 +386,40 @@ PMOD_EXPORT void o_cast_to_string(void) get_name_of_type(sp[-1].type)); } } - return; + break; case T_INT: +#if 0 sprintf(buf, "%"PRINTPIKEINT"d", sp[-1].u.integer); +#else + { +#define sizeNUM(v) (8*sizeof(v)*4/10+1+1) + INT32 org; + char buf[sizeNUM(org)]; + register char*b = buf+sizeof buf-1; + register unsigned i; + org = sp[-1].u.integer; + pop_stack(); + *b-- = '\0'; + i = org; + + if( org < 0 ) + i = -i; + + goto jin; + do { + i /= 10; +jin: *b-- = '0'+(i%10); + } + while( i >= 10 ); + + if( org<0 ) + *b = '-'; + else + b++; + push_text( b ); + } +#endif break;
case T_ARRAY: @@ -462,18 +491,18 @@ PMOD_EXPORT void o_cast_to_string(void) pop_stack(); push_string(s); } - return; + break;
- case T_FLOAT: + case T_FLOAT: { + char buf[sizeNUM(double)*2]; sprintf(buf, "%f", (double)sp[-1].u.float_number); + push_text(buf); break; + }
default: Pike_error("Cannot cast %s to string.\n", get_name_of_type(sp[-1].type)); } - - sp[-1].type = PIKE_T_STRING; - sp[-1].u.string = make_shared_string(buf); }
PMOD_EXPORT void o_cast(struct pike_type *type, INT32 run_time_type)
Some notes:
case T_INT: +#if 0 sprintf(buf, "%"PRINTPIKEINT"d", sp[-1].u.integer); +#else
- {
+#define sizeNUM(v) (8*sizeof(v)*4/10+1+1)
^^^^^^^^^^^^^^^^^^^^ It'd be nice with a comment explaining the various integers above.
INT32 org;
^^^^^ You should probably have INT_TYPE here; otherwise there'll be problems with semi-large integers on 64-bit architectures.
char buf[sizeNUM(org)];register char*b = buf+sizeof buf-1;register unsigned i;
^^^^^^^^ unsigned INT_TYPE here (same as with org above).
org = sp[-1].u.integer;
[...]
- case T_FLOAT:
- case T_FLOAT: {
- char buf[sizeNUM(double)*2];
^^^^^^^^^^^^^^^^^ Is this safe? What about the exponent?
sprintf(buf, "%f", (double)sp[-1].u.float_number);
[...]
-- Sincerely, Stephen R. van den Berg.
Henrik Grubbstr?m (Lysator) @ Pike (-) developers forum wrote:
+#define sizeNUM(v) (8*sizeof(v)*4/10+1+1)
^^^^^^^^^^^^^^^^^^^^It'd be nice with a comment explaining the various integers above.
:-). I'll have to rethink what they mean, I figured this out a very long time ago, and simply reused it time and again.
INT32 org;^^^^^You should probably have INT_TYPE here; otherwise there'll be problems with semi-large integers on 64-bit architectures.
Well, for one, what is the largest integer type to be expected in the svalue (without bignums)? Is that INT_TYPE instead of INT32?
- case T_FLOAT: {
- char buf[sizeNUM(double)*2];
^^^^^^^^^^^^^^^^^Is this safe? What about the exponent?
The number of decimal digits plus one to represent an integer of type x is:
sizeNUM(x)
This implies that to represent an equal number of bits in the mantisse and exponent is:
sizeNUM(mantisse in bytes) + sizeNUM(exponent in bytes)
and size (mantisse+exponent) in bytes is sizeof(double) hence by approximating for a double that the number of digits needed to represent it is:
sizeNUM(double) * 2
should normally overestimate the total maximum stringsize by roughly a factor of two, which is a safe margin for unexpected floating point formatting options. Then again, we could start using snprintf there.
Well, for one, what is the largest integer type to be expected in the svalue (without bignums)? Is that INT_TYPE instead of INT32?
My Pikes has been running 64 bits there for around 8 years... So yes.
sizeNUM(double) * 2
should normally overestimate the total maximum stringsize by roughly a factor of two, which is a safe margin for unexpected floating point formatting options. Then again, we could start using snprintf there.
Not exactly.
strlen((string)1e100);
(1) Result: 108
108 >> 27.
Fun fact:
strlen((string)1e200);
*** stack smashing detected ***: pike terminated ======= Backtrace: ========= /lib/libc.so.6(__fortify_fail+0x37)[0x2aff8e019607] /lib/libc.so.6(__fortify_fail+0x0)[0x2aff8e0195d0] pike(o_cast_to_string+0x558)[0x4ea998]
(latest CVS)
And I'm sure you'd like to use FLOAT_TYPE since it can actually be set to long double. (I'm not sure how well it works, but it's a theory.)
Mirar @ Pike developers forum wrote:
sizeNUM(double) * 2
should normally overestimate the total maximum stringsize by roughly a factor of two, which is a safe margin for unexpected floating point formatting options. Then again, we could start using snprintf there.
Not exactly.
strlen((string)1e100);
(1) Result: 108
108 >> 27.
That's a bignum which is not handled by the T_INT case.
Fun fact:
strlen((string)1e200);
*** stack smashing detected ***: pike terminated ======= Backtrace: ========= /lib/libc.so.6(__fortify_fail+0x37)[0x2aff8e019607] /lib/libc.so.6(__fortify_fail+0x0)[0x2aff8e0195d0] pike(o_cast_to_string+0x558)[0x4ea998]
(latest CVS)
This would indicate that the %f format specifier is indeed rather unsuitable for a generic cast.
And I'm sure you'd like to use FLOAT_TYPE since it can actually be set to long double. (I'm not sure how well it works, but it's a theory.)
Ok.
Stephen R. van den Berg wrote:
Mirar @ Pike developers forum wrote:
strlen((string)1e100);
(1) Result: 108
108 >> 27.
That's a bignum which is not handled by the T_INT case.
Sorry, it's a float/double of course (thinking too fast earlier).
Another point here I've been annoyed at:
I don't think "%f" is what you exactly want when you cast to string. "%.*g" where * is the amount of digits you can have in the mantissa in (plus some) or so would probably be better in almost all cases. "%f" is typically bad in many cases:
(string)1e-20;
(9) Result: "0.000000"
You added a call to snprintf before checking that in so now the Windows build is broken:
operators.obj : error LNK2019: unresolved external symbol _snprintf referenced in function _o_cast_to_string pike.exe : fatal error LNK1120: 1 unresolved externals
WTF! Do not check in optimizations! I assumed we where talking 7.9 here but apparently I should stop assuming people have some sort of sense. I'll give up my hopes of getting a beta out today.
Let me say this again: If you think something is neat it should not be checked in. Period. Stop using the repository like a bloody sandbox. And stop refactoring code. Generally just stop fiddling with things or I will have to make the repository read only.
Now back that out.
Peter Bortas @ Pike developers forum wrote:
WTF! Do not check in optimizations! I assumed we where talking 7.9
It's a simplification and a bugfix against buffer overflow, which is not just an optimisation for the heck of it, IMHO that increases stability. It depends on which label you put on it.
Now back that out.
It's backed out.
Pike now crashes in verify on my machine, again.
Please anti-back at least the bugfix part?
I too have no problem with a well-tested bugfix, but the snprintf call part of the code reorganization that broke Windows build should be saved for 7.9 in my opinion.
Oh right, I spotted that, yes. snprintf doesn't exist there. But could we have some sort of bugfix?
I'd be happy with a %.10g instead of %f. (Anyone against?)
Mirar @ Pike developers forum wrote:
Oh right, I spotted that, yes. snprintf doesn't exist there. But could we have some sort of bugfix?
I'd be happy with a %.10g instead of %f. (Anyone against?)
IMO, as a minimum patch I'd suggest:
sprintf(buf, "%.*g", MAX_FLOAT_SPRINTF_LEN, (double)sp[-1].u.float_number);
It's reasonably accurate, and reasonably safe.
Well, noone seems to protest very loudly, so please check in that fix.
Mirar @ Pike developers forum wrote:
Well, noone seems to protest very loudly, so please check in that fix.
It's in, but something else still causes the selftests to fail.
Mirar @ Pike developers forum wrote:
Unfortunately... But it doesn't seem to be related to cast to string, at least?
It's on mast or grubba's turf, I believe.
Mirar @ Pike developers forum wrote:
Unfortunately... But it doesn't seem to be related to cast to string, at least?
It's on mast or grubba's turf, I believe.
Indeed, it was a rather obscure bug; the code generator for the predef::`+() (generate_add()) didn't adjust the calculated stack depth.
-- Sincerely, Stephen R. van den Berg.
Peter Bortas @ Pike developers forum wrote:
WTF! Do not check in optimizations! I assumed we where talking 7.9
Oh, and BTW, it was intended for 7.9, but then someone checked in some tests which broke 7.8, and those tests just happened to be fixed by the changes I was working on; so I figured fixing the testresult by checking in the simplified and debugged code would be an improvement.
pike-devel@lists.lysator.liu.se
-
Henrik Grubbstr�m (Lysator) ＠ Pike (-) developers forum -
Jonas Walld�n ＠ Pike developers forum
-
Mirar ＠ Pike developers forum
-
Peter Bortas ＠ Pike developers forum
-
Stephen R. van den Berg