Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
Hi, [EMAIL PROTECTED] writes: but you continue to state that it makes the code cleaner which it clearly doesn't. I say using a type that represents the actual type of the value closely is a feature and not a bug. What wrong about seing: Hey, this value is supposed to be unsigned? This is a cleanup like using enums instead of magic values. if we'd use a programming language that would properly support ranged types, it'd be a cleanup. C however doesn't assure that. If the program would throw a signed exception if a negative value is ever assigned to an unsigned variable, we'd indeed be able to catch bugs and declaring variables as unsigned would make sense. Since this is not the case, the use of unsigned variables is dangerous without giving any real advantages. it gives a small improvement and is probably a valid optimization in this place. That's also not the point. You doubted that using unsigneds/signeds doesn't make a difference and I told you that it might not make a difference on all platforms and compilers but definitely does in not so few cases. your example involved a modulo operation. Modulo is only properly defined on positive values so it definitely makes sense to use an unsigned variable here. But then modulo operations are rare, subtractions are much more frequent and they will give unexpected results if unsigned variables are involved. The difference to the changes I reverted is that this is a very local change. The function is small and the variable is local. It is easy to decide that using an unsigned integer here does not have any unwanted side effects. Sure, but since you're breaking GIMP anyways what's the problem with doing it correctly here? BTW: I tested the afffected functions (including the brushes that you stated would break) and at least there is no obvious breakage but if someone is careing about the code it can only get better not worse. you can't have tested all possible usage patterns since there are way too many. Your changes however did affect mostly headers. This means the values are used in a lot of places and it is almost impossible to check them all. Even if you would check them all, a future change to the code might introduce a bug because the one who did the change didn't think about the unsignedness of the variable (which is defined somewhere else). It will introduce bugs but it will also make a few older bugs obvious and correctable. the point is: how are we supposed to catch them? If the compiler would give us hints like it does for wrong usage of consts and enums, great. However, this is not the case unfortunately. Since the code is very much in flux right now, a lot of these changes are going to happen in the future. I don't get it. distances are for instance per definition always positive if we have a concrete list of types that are always signed or unsigned and we stick to it then we'd have more then now: A documentation and guide for datatypes. distances are a good example since they can very well be negative: dist = a - b; this perfectly valid code will awfully break if b is larger than a and dist is defined unsigned. For the CPU and the compiler it wouldn't make any difference, only the outcome would be wrong. In the worst case the problem only becomes apparent under rare circumstances and is not even caught during the development process. Yes, this might happen though I consider that unlikely. then have a look at some of the bugs reported in plug-ins lately. They have been reported years after the first release and didn't get caught because they only showed up when the plug-in was used on small images (width 64 || height 64). Such a bug could very well be caused by the use of an unsigned variable. Salut, Sven ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On Wed, Dec 05, 2001 at 01:11:56AM +0100, [EMAIL PROTECTED] wrote: I say using a type that represents the actual type of the value closely is a feature and not a bug. What wrong about seing: Hey, this value is supposed to be unsigned? Because that's not what unsigned does in C. Unsigned is a promise to the compiler by the programmer; don't make the promise if you're not certain you can. The compiler doesn't enforce type safety with respect to unsigned (except very trivially). -- I love catnip mice. It's why I chew their heads off. They're good for breakfast. ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
Hi, [EMAIL PROTECTED] writes: IMHO not because you're abusing the real value for errors and thus one variable for 2 purposes which is a bad idea and using signed integers is dragging down performance. It is also a bad idea to use signed integers for most loops for example; unsigned int i; for (i = 0; i width; i++) foo (); tends to be more efficient in some cases (depending on the use of i) then when categorily using signed ints. this is not true. Please stop spreading this nonsense about unsigned integers being more performant than signed ones. Go and write yourself a simple benchmark for the code you mentioned above and you will notice that it doesn't make any difference at all. you also removed a good bunch of debugging code that might still be needed (Kelly is working on this code) and unsignified lots of integers, which is why I choose to revert the changes in this file in a whole. The debugging cruft is pretty worthless, something to be added if really needed. yes, but it is not nice to remove it while Kelly was working on this part of the code. I do hope you will post the tile_manager_get_tile_num() part of your change to the list. Actually it makes only sense when using unsigned variables because otherwise the optimisation is mostly gone. If we can agree that nrows and ncols are unsigned then I'm willing to post the patch here If code makes assumptions about parameters (like for example assuming that width is 0), there has to be code like g_return_if_fail() to check this assumption. Defining width as an unsigned integer would also guarantee this, but it unneededly introduces a possibility for a bug if width is ever used in a subtraction. Not in the subtraction itself, only if the destination is also unsigned and if one expects a positive result and gets a negative one there's a bug in the code anyway. the side effects of unsigned integers are not what people are used to think about when designing an algorithm. You are changing the mathematical base in an unneeded and hardly foreseeable way. Code that looks correct and used to work fine, suddenly starts to behave wrong. I doubt that you've checked each and every usage of the variables you changed lately. IMO using unsigned integers is a concept from the stone-age of programming when CPUs and compilers used to be crap and the use of unsigned values gave a real speed gain. FWIW I've converted lots of parameters and variables in paint-funcs to unsigned and I've experienced a 2,5% object code reduction in this part of the GIMP and since the calculations were simplified by the compiler (proven by assembly examination) it's for sure also faster though it's quite hard to profile this code since there's no benchmark generating reproducible results. what are you aiming for, GIMP-embedded? You are speaking about ~1.3 KB of object size reduction. This is ridiculous and there are certainly other parts of The GIMP that can be made much slimmer with a less error-prone approach. I also doubt that the code size reduction or speed improvement originates in the use of unsigned variables. It would surprise me very much if it would make any difference for the compiler. Signed and unsigned variables are treated the same in almost all situations. If you are about to optimize parts of the GIMP, the first thing you should do is write a test suite and benchmarking facility for this part of The GIMP. No optimization should be allowed without such a test suite. I still think GIMP is too fat and the code in parts too ugly. Though my changes in general just show small improvements I usually also fix stylistic problems and find unoptimal code and bogosity during the audits. Having a clear view what a function does and which parameters it expects is pretty important and correctly typing their arguments is a good step forward in achieving quality code which always runs at maximum speed. Well, going through the code like a berserk doesn't really help here. First, it is way too early to start optimizing the code in a whole. Then, only code that is finished, working and stable should be optimized to make it possible to catch errors introduced by the optimizations. Last, only code that showed up in a profile is worth to be optimized. What needs to be done at the moment is to give the GIMP core a well-defined structure, to document and to debug it. A few small encapsulated areas like the paint-funcs can be optimized now and if benchmarks proove that the use of unsigned variables speeds things up, then use them there for god's sake. But please, stop going over other peoples code that you don't understand and stop applying your so called optimizations to it. Thank you. Salut, Sven ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On 3 Dec, Robert L Krawitz wrote: Why? If a function is explicitly documented as returning an error, it's the caller's responsibility to handle it. Right. The callee often doesn't know the high level context to handle it in a useful fashion. I'm talking about nested function calls. If a function deep inside fails it should be handled as quickly as possible instead of propagating it through the code. -- Servus, Daniel ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On 3 Dec, Robert L Krawitz wrote: By how much? Depends on the code and the compiler. And the range I'm talking about is usually between 0 and 50% improvement in both code and size. If it can't be measured, it's probably not enough to be worthwhile. Aside from the gains it's IMHO also cleaner. The gcc and linux kernel people handle it the same way btw. And if you use explicit error returns (which may require an additional pointer to be passed in), you'll quickly eat up any performance gain you might achieve. No, the tile-manager change I did shows this pretty clear. Although using another parameter the object size is smaller here. If I get around to do so I'll check the difference for a few more architectures here. Using signed integers also catches a particularly common error case (subtracting a value from a smaller value) that otherwise has to be checked for explicitly, which is also going to require more instructions (and even worse on modern processors, branches). I doubt it. If negative numbers are likely to happen then the code has to check for it anyway and thus the unsignedness of the single parts of the calculation don't make any difference. And further modern processors have conditional instructions which can be exactly used for such kind of expressions and the more simple the check the easier it is for the compiler to apply them. It is easier to check any type agains 0 for example than against negativeness and this part also plays a role when returning 0 or non-null instead of a negative value. Anyway, integer arithmetic (additions and subtractions) is usually one cycle (plus however long it takes to retrieve the operands and instruction from memory, cache, whatever) on modern processors, so it's hard for me to see how unsigned arithmetic is going to help. That's correct, the CPU doesn't care about signedness. But by telling the compiler that a value is unsigned it can apply additional optimisation and it also simplifies some calculations considerably. For example if you shift a signed value it has to generate code to take care of the sign and similar with some logic operations. Fine, but a = c - d; if (a 0) abort() is likely to be more efficient than if (c d) abort() else a = c - d As this stands it is absolutely correct. However this is only a part of the picture. The question is: What will happen in the rest of the code? Also no one would prevent you from making a signed if you suspect a to be negative. There's a lot of code where it is given that some value is positive for example because it is used in loops which start from 0 all over the place. I'm not argueing about signed values in general but about variables which are by purpose defined to be unsigned. And yes, maybe there's a bug in the code, but isn't it better to have a reasonably easy way of catching it? If you put assertions in the code, they can be compiled in during debugging, and then turned off (thereby not consuming any extra time) when the code is solid. One may or may not like the way Linus Torvalds codes but his programming has definitely quite some style and elegance and one of his points is only to have checks where you expect errors to happen instead of all over the place. Putting assertions in there to catch problems in the development phase is definitely a good idea and once the code is stable they'll ressolve into nops and all that's left is efficient code; that's how it should work. Get some numbers first before worrying about micro-optimizations like this. I would if I could. Code that looks simpler isn't necessarily faster, and in Trust me, if I see the assembly I can tell you which one is faster and by which magnitude. As you already said most arithmetic calculations are one-cycler on modern CPUs and seldomly have sideeffects while memory accesses in general are slow; if the assembly shows fewer memory accesses AND fewer calculations while the branches are unaffected there would have to been serious pipeline issues (because of dependencies) in the code to make it slower. particularly it may not be *significantly* faster. You're better off looking for hot spots in the code; if this isn't a hot spot, it doesn't matter what you do. Oh, paint-funcs are a hotspot, it's quite easy to drag a fast machine down by painting with a big brush in a transparent layer for instance; most of the time is then spent in the combine_* functions which is (as it is now) slow and I'm looking to speed that up by changing the algorithm; still the basic calculations will remain the same so speeding them up while brushing up the code is IMHO not a bad idea. Anyway, we're talking about stuff here that's partially done. I've done some stuff like this in Gimp-print (although I haven't found unsigned vs. signed to matter very much, unless I can replace a division by a power of two by a right shift, in which it's sometimes worthwhile to take the branch penalty), but
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On 4 Dec, Sven Neumann wrote: this is not true. Please stop spreading this nonsense about unsigned integers being more performant than signed ones. Go and write yourself a simple benchmark for the code you mentioned above and you will notice that it doesn't make any difference at all. Heck, you clearly demonstrate that you've no idea what you're talking about. unsinged types are not necessarily leading to faster code but they sometimes enable better optimisation possibilities for the compiler because every single fact the compiler knows aditionally about types or functions like pureness, constness or staticness (of functions) will lead to additionally hints the compiler notes in is internal representation and will lead to more optimal code for the narrowed purpose. Check the code if you don't believe it. yes, but it is not nice to remove it while Kelly was working on this part of the code. Yes, that wasn't nice, apologies to Kelly. though I can imagine better forms of debugging code than commented out printf's the side effects of unsigned integers are not what people are used to think about when designing an algorithm. You are changing the mathematical base in an unneeded and hardly foreseeable way. Code that looks correct and used to work fine, suddenly starts to behave wrong. I doubt that you've checked each and every usage of the variables you changed lately. I was about to check in another followup patch which would have cleared some of the maybe-problems this might have created though you can scratch this idea now. IMO using unsigned integers is a concept from the stone-age of programming when CPUs and compilers used to be crap and the use of unsigned values gave a real speed gain. Right, we should just use signed values and scratch all inlines, and consts since compilers are clever enough to figure it out anyway. Are you really believing what you're saying? When Marc and I where experimenting at Mitchs place we figured out that providing additional hints to gcc via non-portable attributes gave the compiler quite some chances to optimise better it was you who said that we might want to add this in macroized form to GIMP and now you're telling that compilers are clever enough anyway to not get any advantages from such hints? what are you aiming for, GIMP-embedded? Actually yes. You are speaking about ~1.3 KB of object size reduction. This is ridiculous and there are certainly other parts of The GIMP that can be made much slimmer with a less error-prone approach. I also doubt that the code size reduction or speed improvement originates in the use of unsigned variables. This was part of a general cleanup to better understand where the flow can be optimised, it wasn't meant to be the huge-improvement-for-free change but I also had cleaness and optimisation in mind when doing so. It would surprise me very much if it would make any difference for the compiler. It surprises me that you're surprised instead of checking the situation in real life. Signed and unsigned variables are treated the same in almost all situations. I'd restrict that to many situations. If you are about to optimize parts of the GIMP, the first thing you should do is write a test suite and benchmarking facility for this part of The GIMP. Bad luck, not from me. No optimization should be allowed without such a test suite. I disagree, there are for sure many algorhythms which can be optimised complexity wise and with micro changes but flow changes can hardly be benched without a macro benchmark and since you object to branches in CVS there's hardly any way this will ever happen since once cannot do major surgery in the flow without having a change to do it in smaller steps. Anyway, since no benchmark is available and the hooks to add one are rather unsatisfying one will have to use indicators to optimise. I'm personally using gprof, gcov, gcc -save-temps and diff to tell whether an improvement is there. You don't want optimisation - then go without it; not my problem Well, going through the code like a berserk doesn't really help here. Great, so this is your word to stop doing so as well? First, it is way too early to start optimizing the code in a whole. I disagree, for microscopic changes out of nowhere - yes, but I had some bigger plans in mind. Then, only code that is finished, working and stable should be optimized to make it possible to catch errors introduced by the optimizations. Last, only code that showed up in a profile is worth to be optimized. You're telling me that? Surprising, I remember it was you who asked me how to profile code, right? What needs to be done at the moment is to give the GIMP core a well-defined structure, to document and to debug it. I have a smallish patch to document the functions and structures I've been modifying, but since cleaness doesn't seem to be a valid point except when it comes to GObjects - so be it. A
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On Tue, Dec 04, 2001 at 02:17:06PM +0100, [EMAIL PROTECTED] wrote: agains 0 for example than against negativeness and this part also plays a role when returning 0 or non-null instead of a negative value. Sorry, but before you continue with all this, ehrm, wrongness, would you please first check what you are talking about? Can you give us a single example of such a cpu? No cpu linux runs on has this property, btw. The reason nobody wants to talk reasonable with you is that most of what you claim is simply that: wrong. People cannot trust what you say when they cna trivialy verify most of you claims as wrong. If you would only give the points that _are_ true, then people will be much more open in discussing optimizations. For example if you shift a signed value it has to generate code to take care of the sign and similar with some logic operations. Again, not on any cpu that linux runs on. Trust me, if I see the assembly I can tell you which one is faster and by which magnitude. How can we trust you if most of the easily-verifiable claims of yourself are simply untrue? *please* this is not a with-hunt. *please* re-adjust your attitude. when so many people tell you that you are wrong you could at least check wether tzhis is true. -- -==- | ==-- _ | ---==---(_)__ __ __ Marc Lehmann +-- --==---/ / _ \/ // /\ \/ / [EMAIL PROTECTED] |e| -=/_/_//_/\_,_/ /_/\_\ XX11-RIPE --+ The choice of a GNU generation | | ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
[Gimp-developer] Re: your so called optimizations and why we don't like them
Hi Rebecca, [EMAIL PROTECTED] (2001-12-04 at 1737.58 +0100): Can't we all bask in the gimp love? Not to split hairs, but isn't it gimplove with no space? carol ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On Tue, Dec 04, 2001 at 11:34:59AM +0100, Sven Neumann wrote: the side effects of unsigned integers are not what people are used to think about when designing an algorithm. You are changing the mathematical base in an unneeded and hardly foreseeable way. Code that looks correct and used to work fine, suddenly starts to behave wrong. I doubt that you've checked each and every usage of the variables you changed lately. There are places in tiles, tile managers, and pixel regions that use negative numbers in unexpected ways (for example, theres's a spot in the shapeburst code that uses a negative rowstride to go backwards through an image buffer). Changing these variables to unsigned will obviously break things. If you are about to optimize parts of the GIMP, the first thing you should do is write a test suite and benchmarking facility for this part of The GIMP. No optimization should be allowed without such a test suite. I can't agree more with this. Profile the GIMP before you try optimizing it. You may be surprised to discover where the GIMP spends most of its CPU time. And trying to optimize a moving code base is just plain stupid. Kelly -- I love catnip mice. It's why I chew their heads off. They're good for breakfast. ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On Tue, Dec 04, 2001 at 01:39:36PM +0100, [EMAIL PROTECTED] wrote: I'm talking about nested function calls. If a function deep inside fails it should be handled as quickly as possible instead of propagating it through the code. Uh, this is C, not Scheme. We don't throw exceptions. Calling gimp_fatal_error is not an option in most cases (most errors are not fatal). The correct response to an error condition is to return an error indication in accordance with your API. Upstream callers are responsible for checking error returns except for errors which cause transfer-of-control with longjmp or exit. -- I love catnip mice. It's why I chew their heads off. They're good for breakfast. ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On 4 Dec, Sven Neumann wrote: Check the code if you don't believe it. Sorry, but that's exactly what I did before I posted the reply and I'm asking you to do that too. A simple benchmark prooves that the example you gave is wrong since the use of unsigned variables doesn't make any difference. But you also do read my mails, do you? And I said clearly that it might make a difference in larger functions not that it necessarily betters anything. Example? Ok, here you go, I subsituted an gint which is used for a clearly positive range (1; length) by an guint. This is an arbitrary example and doesn't give to much of a benefit though the function is still simple. --- paint-funcs.c.orig Thu Nov 29 14:17:47 2001 +++ paint-funcs.c Tue Dec 4 21:53:49 2001 @@ -343,7 +343,8 @@ gdouble sigma2; gdouble l; gint temp; - gint i, n; + guinti; + gint n; sigma2 = 2 * sigma * sigma; l = sqrt (-sigma2 * log (1.0 / 255.0)); will lead to that difference in PPC assembly: --- paint-funcs.backup.sTue Dec 4 21:52:45 2001 +++ paint-funcs.s Tue Dec 4 21:53:57 2001 @@ -2298,7 +2298,7 @@ .align 3 .LC5: .long 0x4330 - .long 0x8000 + .long 0x0 .section.text .align 2 .typemake_curve,@function @@ -2361,7 +2361,6 @@ .L1202: mullw 0,30,30 neg 0,0 - xoris 0,0,0x8000 stw 0,12(1) stw 26,8(1) lfd 1,8(1) This is just to prove you that it can make a difference. I haven't said that and you will have noticed that we've added lots of consts to the code recently. And this is a good thing. It might break stuff exactly like the unsigned case but it will lead to better code and is much cleaner. I didn't say optimization is a bad thing. I just doubt that the use of unsigned variables is worth the risk of introducing bugs it certainly bears. There are exceptions, but definitely not in the UI part of the code you've changed recently. I wouldn't have touched the UI code if it was more seperate from the rest. Bad luck, not from me. that, I call ignorance. That I'd call lack of time and interest. No, since we announced that cleaning up the code is the major goal for 1.4. The way we target this is (in this order): 1. Move files into subdirectories that define subsystems and reduce the dependencies between these subsystems. 2. Clean up the code so it uses a common coding-style. 3. Make the code bug-free (this should involve writing test suites for the various subsystems). 4. Profile it. 5. Optimize it. Since we aren't even done with parts 1 and 2; can't you understand that we'd prefer not to start with 5 yet?! Sure, but the changes were also part of your point 2, and it's never to early to work on cleanliness. I didn't object to any of your modifications in the paint-funcs. Great, but still I'm rather unhappy about the recent happenings and that all of it happened in the public; you don't even respond to private email. I'd say it's up to you at this point to proove your arguments. I've done my homework and benchmarked the use of unsigned vs. signed. Don't even try to catch a difference using a synthetic microbenchmark; do it on real code and you will see that it makes a difference. Just to notice another thing before people start complaining, the example above was done on PPC with gcc-2.95.4, any other architecture or compiler might have different results but it's important to add that any detoriation is a bug in the compiler so if done properly there are only advantages to be had from it. -- Servus, Daniel ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
[Gimp-developer] Re: your so called optimizations and why we don't like them
On 3 Dec, Sven Neumann wrote: you recently checked in a huge change to GIMP that we (Mitch and me) have been very unhappy with. In the meantime, I have gone through the hassle of looking at every single line of your changes and I have reverted most of it. Here are the reasons, why we think your optimizations don't make sense and, even worse, are dangerous: BTW: It would have been a single command for me to revert the changes but since you decide to work everything on your own. While unsigned variables can speed things up under very special circumstances, Not special, in many circumstances. they bear the danger of introducing subtle bugs that are almost impossible to find. A good example is found in pango/docs/TEXT/coding-style (a recommened read, btw.): If width is unsigned and 10, then: int new_width = MAX (width - 15, 1); produces 4294967291, not 1. This is clearly a bug in the code. There are sizes which are by definition unsigned and code computes an negative value this is about as harmful as an big positive one. Also, in lots of places you've changed, the value -1 was used to indicate an unset value (width and height of GimpPreview for example). Shouldn't be. I was about to go even further and change all false uses of such values. What you imply is that using unsigned types for unsigned values is wrong because you also use them for errorindication and this (together with magic constants) is about most of the evilness of programming. use bitfields in structs where possible this is an optimizations of structs sizes. It does make sense to use bitfields for boolean fields if large numbers of this struct are allocated. For rarely used objects the gain in memory usage is neglible however. It's not only the memory consumption, it's also the addressing which is much faster (normally) for bitfields because one can read several states at once while the access is realised with masking instead of several memory reads (or writes) and as such using bitfields has several huge advantages: - Less memory accesses (and all the results thereof) - Smaller objects because of less code - Implied error checking against abuse by the compiler My only worry here was that some compilers don't support them but since you started using them in other files I didn't see any problem with this. Since the change makes the code less readable it should be applied with care. I disagree strongly here. The code itself doesn't change and structure definitions tend to win when it comes to readability here. The change again also bears the possibility of introducing bugs. I see it the other way. It bears the possibility to detect unseen bugs. This happens when values others than TRUE (1) and FALSE (0) are assigned to struct fields defined as unsigned int : 1. What do you expect to happen then? For this reason, this change should only be done late in the development cycle. Once the API is settled and the code is stable, we can think about converting booleans in frequently used structs to bitfields. At the very moment, it doesn't make sense at all. Well, your thought. you changed code from gimp-be_verbose = be_verbose ? TRUE : FALSE; to gimp-be_verbose = be_verbose; This is wrong, especially if a bitfield is used. I don't think I need to explain the reason. It doesn't even make sense when NOT using bitfields but I definitely see no problem when doing so. Please don't always assume I see what you mean by telling this is wrong; if you see a problem here tell me so. Daniel, would you please from now on inform us about the changes you are working on and send patches to this mailing list instead of applying your work to CVS without asking. It took me hours to revert your changes and I don't want to have to go through this again. If you think I reverted too much (which I'm sure has happened in a few places), please send the relevant changes in small patches so we can discuss them here. You not being able to use CVS efficently doesn't make me a bad person. Maybe you would have saved you a lot of time if you simply asked me to revert the (still correct) changes. Anyway, since you seem to like to develop on your own because you know everything better anyway; feel free to do so. I don't care whether correct code shines off your ass or you have the Saft but putting embargos up to me because you don't understand what I'm trying to achieve is just likely to piss off another of the few coders left. BTW: Not that I've never corrected any of yours or Mitches code resulting from a thinko or type... It's up to you. -- Servus, Daniel ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
[EMAIL PROTECTED] writes:
If width is unsigned and 10, then:
int new_width = MAX (width - 15, 1);
produces 4294967291, not 1.
This is clearly a bug in the code. There are sizes which are by
definition unsigned and code computes an negative value this is
about as harmful as an big positive one.
This is not a bug in the code, it's only a bug if the code uses
unsigned. Why on earth should we introduce side conditions that make
using perfectly ok code like above buggy? Using signed is just
defensive programming, using unsigned introduces side effects that are
hard to find.
Also, in lots of places you've changed, the value -1 was used to
indicate an unset value (width and height of GimpPreview for
example).
Shouldn't be. I was about to go even further and change all false uses
of such values. What you imply is that using unsigned types for unsigned
values is wrong because you also use them for errorindication and this
(together with magic constants) is about most of the evilness of
programming.
I don't see why the type preview size which is defined as
size of the preview if = 0, or undefined otherwise is bad.
What special value would you introduce instead?
This happens when values others than TRUE (1) and FALSE (0) are
assigned to struct fields defined as unsigned int : 1.
What do you expect to happen then?
The following will happen:
struct eek
{
guint argh : 1;
};
void
foobar (struct eek eek,
gboolean baz)
{
eek.argh = baz;
}
Now calling foobar(my_eek, 2);
will result in my_eek.argh being 0 (the least significant bit of 2).
Assigning to a gboolean (which is a *signed* int) will assign 2
and still be TRUE in the C sense.
you changed code from
gimp-be_verbose = be_verbose ? TRUE : FALSE;
to
gimp-be_verbose = be_verbose;
This is wrong, especially if a bitfield is used. I don't think I need
to explain the reason.
It doesn't even make sense when NOT using bitfields but I definitely see
no problem when doing so. Please don't always assume I see what you mean
by telling this is wrong; if you see a problem here tell me so.
It makes sense and kindof *must* be used for unsigned bitfields. See above.
Furthermore, I don't see a reason for being pissed of. If you announced
what you were about to do we could have discussed it beforehand.
ciao,
--Mitch
___
Gimp-developer mailing list
[EMAIL PROTECTED]
http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
Hi, [EMAIL PROTECTED] writes: However I see much room for improvement here. The values I changed to unsigned are likely meant to be unsigned like sizes, distances or heights or widths. And those are frequently used in loops like int i; for (i = 0; i width; i++) foo (); if we were to expect negative values in this case we'd be really doomed and as such I consider it a feature to be warned by the compiler if a calculation is definitely underflowing or tests are useless. Negative values do have a right to exist when it comes to coordinates but using signed types for error reporting AND value represantation at the same time is likely to be errorprone. Using them for error reporting is definitely a bad idea. Using a negative value to indicate that a value has not been set and needs to be computed is IMO a reasonable usage. I propose to do it the following way: gboolean do_something (guint *value, ...) (means passing a pointer to the place the result should be written to) instead of gint do_something (...) and returning a negative value in case of failure yes! I've applied exactly this scheme to tile_manager_get_tile_num () in tile-manager.c and together with replacing the ongoing errorchecking throughout the callees was able to save a whooping 480 bytes in object size on PPC and got faster code while (IMHO) making the code clearer. you also removed a good bunch of debugging code that might still be needed (Kelly is working on this code) and unsignified lots of integers, which is why I choose to revert the changes in this file in a whole. I do hope you will post the tile_manager_get_tile_num() part of your change to the list. I do because one has to constantly check for errorvalues and there lies one problem; we have several places in the GIMP where return values are checked for errors and then passed down to another function where they are checked again while there are also places where we don't have error checking at all. Do we? We do have a lot of g_return_val_if_fail() statements all over the place, but this is for debugging purposes only and should be disabled for production code. No such call should ever be triggered; if it is, there's a bug. For this reason, the caller does not need to check for values returned by g_return_val_if_fail(). The whole purpose of these statements is to catch bugs early. If code makes assumptions about parameters (like for example assuming that width is 0), there has to be code like g_return_if_fail() to check this assumption. Defining width as an unsigned integer would also guarantee this, but it unneededly introduces a possibility for a bug if width is ever used in a subtraction. Using signed types is unelegant and prevents the compiler from optimising the code better. NULL pointers have the nice property of being easy to check and I think it's a nice idea to have functions return 0 or !0 aka TRUE or FALSE instead of some bogus values because it's clearer and faster. I agree about the return value thing, but I doubt the use of signed or unsigned makes up for any noticeable speed difference expect under very rare conditions. Before any integer is changed to an unsigned value, a benchmark should proove that it really is an optimization and all code using the value needs to be reviewed carefully. Salut, Sven ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On 4 Dec, Sven Neumann wrote: Using them for error reporting is definitely a bad idea. Using a negative value to indicate that a value has not been set and needs to be computed is IMO a reasonable usage. IMHO not because you're abusing the real value for errors and thus one variable for 2 purposes which is a bad idea and using signed integers is dragging down performance. It is also a bad idea to use signed integers for most loops for example; unsigned int i; for (i = 0; i width; i++) foo (); tends to be more efficient in some cases (depending on the use of i) then when categorily using signed ints. you also removed a good bunch of debugging code that might still be needed (Kelly is working on this code) and unsignified lots of integers, which is why I choose to revert the changes in this file in a whole. The debugging cruft is pretty worthless, something to be added if really needed. I do hope you will post the tile_manager_get_tile_num() part of your change to the list. Actually it makes only sense when using unsigned variables because otherwise the optimisation is mostly gone. If we can agree that nrows and ncols are unsigned then I'm willing to post the patch here If code makes assumptions about parameters (like for example assuming that width is 0), there has to be code like g_return_if_fail() to check this assumption. Defining width as an unsigned integer would also guarantee this, but it unneededly introduces a possibility for a bug if width is ever used in a subtraction. Not in the subtraction itself, only if the destination is also unsigned and if one expects a positive result and gets a negative one there's a bug in the code anyway. I agree about the return value thing, but I doubt the use of signed or unsigned makes up for any noticeable speed difference expect under very rare conditions. Before any integer is changed to an unsigned value, a benchmark should proove that it really is an optimization and all code using the value needs to be reviewed carefully. FWIW I've converted lots of parameters and variables in paint-funcs to unsigned and I've experienced a 2,5% object code reduction in this part of the GIMP and since the calculations were simplified by the compiler (proven by assembly examination) it's for sure also faster though it's quite hard to profile this code since there's no benchmark generating reproducible results. I still think GIMP is too fat and the code in parts too ugly. Though my changes in general just show small improvements I usually also fix stylistic problems and find unoptimal code and bogosity during the audits. Having a clear view what a function does and which parameters it expects is pretty important and correctly typing their arguments is a good step forward in achieving quality code which always runs at maximum speed. -- Servus, Daniel ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On 4 Dec, Sven Neumann wrote: Using them for error reporting is definitely a bad idea. Using a negative value to indicate that a value has not been set and needs to be computed is IMO a reasonable usage. On a side note: I found it quite often that the return value is set to something in case it's real value couldn't be computed, this is also some form of error indication which I'd like to avoid: A function fails or succeeds and in either case appropriate steps have to be taken; just propagating the error code down to the original caller in the hope to catch it there is IMHO a bad idea. -- Servus, Daniel ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
From: [EMAIL PROTECTED] Date: Tue, 4 Dec 2001 01:57:26 +0100 (CET) On 4 Dec, Sven Neumann wrote: Using them for error reporting is definitely a bad idea. Using a negative value to indicate that a value has not been set and needs to be computed is IMO a reasonable usage. IMHO not because you're abusing the real value for errors and thus one variable for 2 purposes which is a bad idea and using signed integers is dragging down performance. By how much? If it can't be measured, it's probably not enough to be worthwhile. And if you use explicit error returns (which may require an additional pointer to be passed in), you'll quickly eat up any performance gain you might achieve. Using signed integers also catches a particularly common error case (subtracting a value from a smaller value) that otherwise has to be checked for explicitly, which is also going to require more instructions (and even worse on modern processors, branches). Anyway, integer arithmetic (additions and subtractions) is usually one cycle (plus however long it takes to retrieve the operands and instruction from memory, cache, whatever) on modern processors, so it's hard for me to see how unsigned arithmetic is going to help. If code makes assumptions about parameters (like for example assuming that width is 0), there has to be code like g_return_if_fail() to check this assumption. Defining width as an unsigned integer would also guarantee this, but it unneededly introduces a possibility for a bug if width is ever used in a subtraction. Not in the subtraction itself, only if the destination is also unsigned and if one expects a positive result and gets a negative one there's a bug in the code anyway. Fine, but a = c - d; if (a 0) abort() is likely to be more efficient than if (c d) abort() else a = c - d And yes, maybe there's a bug in the code, but isn't it better to have a reasonably easy way of catching it? If you put assertions in the code, they can be compiled in during debugging, and then turned off (thereby not consuming any extra time) when the code is solid. I agree about the return value thing, but I doubt the use of signed or unsigned makes up for any noticeable speed difference expect under very rare conditions. Before any integer is changed to an unsigned value, a benchmark should proove that it really is an optimization and all code using the value needs to be reviewed carefully. FWIW I've converted lots of parameters and variables in paint-funcs to unsigned and I've experienced a 2,5% object code reduction in this part of the GIMP and since the calculations were simplified by the compiler (proven by assembly examination) it's for sure also faster though it's quite hard to profile this code since there's no benchmark generating reproducible results. Get some numbers first before worrying about micro-optimizations like this. Code that looks simpler isn't necessarily faster, and in particularly it may not be *significantly* faster. You're better off looking for hot spots in the code; if this isn't a hot spot, it doesn't matter what you do. I've done some stuff like this in Gimp-print (although I haven't found unsigned vs. signed to matter very much, unless I can replace a division by a power of two by a right shift, in which it's sometimes worthwhile to take the branch penalty), but I've spent a lot of time with the profiler to actually find the real hot spots. Simply moving constant computations out of loops (into a lookup table, or some such) often saves a lot more time than any of this bit fiddling. I still think GIMP is too fat and the code in parts too ugly. Though my changes in general just show small improvements I usually also fix stylistic problems and find unoptimal code and bogosity during the audits. Having a clear view what a function does and which parameters it expects is pretty important and correctly typing their arguments is a good step forward in achieving quality code which always runs at maximum speed. I agree, but that's exactly why this kind of micro-optimization is premature. You'll get much better results looking for the hot spots first, and understanding exactly what's going on and how you can devise an overall more efficient approach, than by blindly converting signed ints to unsigned and using bit fields. -- Robert Krawitz [EMAIL PROTECTED] http://www.tiac.net/users/rlk/ Tall Clubs International -- http://www.tall.org/ or 1-888-IM-TALL-2 Member of the League for Programming Freedom -- mail [EMAIL PROTECTED] Project lead for Gimp Print/stp -- http://gimp-print.sourceforge.net Linux doesn't dictate how I work, I dictate how Linux works. --Eric Crampton ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
From: [EMAIL PROTECTED] Date: Tue, 4 Dec 2001 02:01:22 +0100 (CET) On 4 Dec, Sven Neumann wrote: Using them for error reporting is definitely a bad idea. Using a negative value to indicate that a value has not been set and needs to be computed is IMO a reasonable usage. On a side note: I found it quite often that the return value is set to something in case it's real value couldn't be computed, this is also some form of error indication which I'd like to avoid: A function fails or succeeds and in either case appropriate steps have to be taken; just propagating the error code down to the original caller in the hope to catch it there is IMHO a bad idea. Why? If a function is explicitly documented as returning an error, it's the caller's responsibility to handle it. The callee often doesn't know the high level context to handle it in a useful fashion. -- Robert Krawitz [EMAIL PROTECTED] http://www.tiac.net/users/rlk/ Tall Clubs International -- http://www.tall.org/ or 1-888-IM-TALL-2 Member of the League for Programming Freedom -- mail [EMAIL PROTECTED] Project lead for Gimp Print/stp -- http://gimp-print.sourceforge.net Linux doesn't dictate how I work, I dictate how Linux works. --Eric Crampton ___ Gimp-developer mailing list [EMAIL PROTECTED] http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer
