Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On 5 Dec, Sven Neumann wrote: > 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. FWIW it might be possible to get this behaviour in future versions of gcc. > you can't have tested all possible usage patterns since there are way > too many. Right, but at least the a gross part functionality works so it can't be as broken as you're suggesting. > 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. You're wrong here. The latest versions of gcc have a vast number of checks that are useful here, just to copy a few from the source: "integer overflow in expression" "large integer implicitly truncated to unsigned type" "negative integer implicitly converted to unsigned type" "overflow in implicit constant conversion" "comparison is always false due to limited range of data type" "comparison of unsigned expression >= 0 is always true" "right shift count is negative" "right shift count >= width of type" "comparison between signed and unsigned" "comparison of promoted ~unsigned with constant" "comparison of promoted ~unsigned with unsigned" "signed and unsigned type in conditional expression" "cast to pointer from integer of different size" "overflow on truncation to unsigned integer" "overflow on truncation to integer" "comparison is always %d due to width of bitfield" Just a quick grep and cut&paste action from a messages that might be related to this topic. Of course this will only catch obvious problems but then again I have some ideas to get even stricter checking. > distances are a good example since they can very well be negative: > dist = a - b; That's a bug. Correct is abs (a - b). > this perfectly valid code will awfully break if b is larger than a > and dist is defined unsigned. Yes, it will. > For the CPU and the compiler it wouldn't make any difference, only the > outcome would be wrong. That's correct, if it really is desired to have negative distances to also encode the orientation then "dist" would have to stay signed and be documented so. > 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. That's a wild speculation. I'm just surprised that the reporter didn't seem to mention that the the images were below some size because that would have made tracking a bug trivial but that's a different issue. -- 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 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: > > 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 5 Dec, Sven Neumann wrote: > 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. And I also say one variable per purpose because everything else is hard follow and likely to break though my patch contained just one patch to address that. > 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. > 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. > 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. > 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. > It is definitely too early to do such global optimizations and I even > vote for never doing them. Wow. > Using unsigned variables locally in parts of the GIMP core that > are executed frequently is still a valid option. Interesting, how do you come to this conclusion? :) > no, it breaks things in a very different manner: the compiler will now > warns us if we try to modify values that we aren't supposed to modify. > Thus we catch bugs at compile time. The compiler will however not warn > us if we use unsigned variables in a way that might cause the value to > become negative. Wrong, if you make it obviously wrong it will issue warnings. If we do it secretly wrong then we'll experience that because it's not of by 1 or by 10 but by >20bit and this is easy to track down. > The result here will be a mis-behaving application > with no indication of where we should look for the problem. I disagree. Obvious problems are much easier to find then sneaky hidden ones. > 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. > the directories that contain pure UI code are called gui and widgets. > Is that not simple enough? The point however is not to disallow > optimizations in the UI and allow them everywhere else. Most parts of > the core are unfinished yet and shouldn't be optimized at this stage. And I tell you again that it's not only optimisation. > we obviously disagree on code cleanliness. IMO the use of unsigned > variables does not make the code any cleaner. Seems so. -- 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 22:08:01 +0100 (CET) --- paint-funcs.c.orig Thu Nov 29 14:17:47 2001 +++ paint-funcs.cTue 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: That's nice. Will that single removed instruction even be noticeable when compared to the square root and the log 2 lines down? >> Bad luck, not from me. > that, I call ignorance. That I'd call lack of time and interest. You're the one trying to prove that it makes a significant difference. It's your responsibility to do the necessary benchmarking. > 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. Fine. Prove it the old fashioned way -- by benchmarking it. -- 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
Hi, [EMAIL PROTECTED] writes: > 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. but you continue to state that it makes the code cleaner which it clearly doesn't. > 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.origThu 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; it gives a small improvement and is probably a valid optimization in this place. 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. 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). Since the code is very much in flux right now, a lot of these changes are going to happen in the future. It is definitely too early to do such global optimizations and I even vote for never doing them. Using unsigned variables locally in parts of the GIMP core that are executed frequently is still a valid option. > > 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. no, it breaks things in a very different manner: the compiler will now warns us if we try to modify values that we aren't supposed to modify. Thus we catch bugs at compile time. The compiler will however not warn us if we use unsigned variables in a way that might cause the value to become negative. The result here will be a mis-behaving application with no indication of where we should look for the problem. In the worst case the problem only becomes apparent under rare circumstances and is not even caught during the development process. > I wouldn't have touched the UI code if it was more seperate from the > rest. the directories that contain pure UI code are called gui and widgets. Is that not simple enough? The point however is not to disallow optimizations in the UI and allow them everywhere else. Most parts of the core are unfinished yet and shouldn't be optimized at this stage. > > 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. we obviously disagree on code cleanliness. IMO the use of unsigned variables does not make the code any cleaner. It is solely part of an optimization process and optimization has to come late and should be applied with lots of care. 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: >> 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
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
On Tue, Dec 04, 2001 at 12:24:34AM +0100, Sven Neumann wrote: > > 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. Please don't make any changes in any tile-related code without talking to me first. That code is touchy and even small, seemingly innocuous modifications can wreak havoc that may be difficult to detect. I am in the process of ripping that code entirely to shreds, so you're really rather wasting your time fiddling with it. Most of it is already subtantially rewritten; I would probably have committed it over the weekend if it wasn't for my network access problems. I realize I've been incommunicado for a few days (fuck you very much, Excite@Home), but that's no excuse for not talking about changes before committing. 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 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 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
Re: [Gimp-developer] Re: your so called optimizations and why we don't like them
Hi, [EMAIL PROTECTED] writes: > 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. 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. > > 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? I haven't said that and you will have noticed that we've added lots of consts to the code recently. The main purpose was to catch bugs though. 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. > > 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. that, I call ignorance. > > 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? 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?! > > 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, > > It did speed up, I just can't tell exactly how much because there is > no benchmark. I didn't object to any of your modifications in the paint-funcs. > Anyway, except there show up some really technical arguments, this is > EOT for me. 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. 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: > 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
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 >
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
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
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
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
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
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
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
Hi, Nathan C Summers <[EMAIL PROTECTED]> writes: > > BTW: It would have been a single command for me to revert the changes > > When Gimp first moved to CVS, and access to the source tree went from a > strong central maintainer to many people with CVS access, the reason I was > told that it would work is that if something is committed that the > community agrees is not what it wants, the whole of the patch can be > reverted with a single command. true, but it wasn't my intention to revert all of it and since Daniel checked in a lot of independent changes in one commit, a single CVS command didn't do the trick. > I don't think that Daniel's reaction was inappropriate at all. Yet > because I'm late for class, I'll let what was said stand for itself. > > What I do think is important is that all major changes (yes, even major > changes by Sven and Mitch) should be discussed before commiting (and even > better, before a significant amount of effort is invested). Preferably, > it should be discussed on both the mailing list and IRC, with any relevant > points made on IRC echoed to the newsgroup (we all get mail and can read > it at our convience, but only Yosh reads the stuff in #gimp 24/7) I agree that we don't communicate changes good enough. We have tried to outline plans for the future of The GIMP here, but the reaction on this mailing list has been minimal. Nevertheless are all tasks currently being worked on supposed to be listed in the file TODO.xml which is used to generate http://developer.gimp.org/gimp-todo.html > I don't know how many times the first time I've heard about some massive > change to the code for the first time in the Changelog, which is usually > something very discriptive like: > * some/file.[ch]: reorganized crap beyond all recognition > (I am exaggerating somewhat. Alas, only somewhat.) A short look at the sizes of the ChangeLog files (pre-1.0, pre-1.2, current) makes me think that ChangeLog entries tend to be quite descriptive nowadays compared to earlier times. But, yes, the communication about GIMP development could and should be much better. > Personally, I can't think of many things that can be done to discourage > developers than this. I can't count the number of times something I have > been working on has been broken by some massive unanounced change. uhmm, that could have been avoided if you'd have announced your plans... > I certainly don't want to disparge those that work actively on Gimp, > especially Sven and Mitch. The contributations that all have made are > very valuable. But I have to wonder if Sven and Mitch don't consider Gimp > to have become thier own personal toy, and this overly harshly toned > critism of Daniel's code is a good example of this. Continuing this > pattern will do nothing to further Gimp in any real way, but will serve > quite well to drive away the few developers that Gimp has left. I've been upset and chose the wrong tone in my mail; probably I shouldn't have sent a mail to this list at all. I did it because I wanted this change to be discussed in public. I apologize if I sounded too harsh. Actually the main goal of the current work is to make the GIMP source a nice place to hack. A better defined architecture should help to keep changes more local. This will allow people to implement new features without forcing them to work their way through the code for half a year before they can dare to touch anything. We are getting closer to our goal of moving the last files from the toplevel application directory to their new places. When this is done, work should start to document the subsystems that have been defined. We will need help to acomplish this goal and it is definitely not our intention to keep people from helping. Of course we are also open to discuss the roadmap for GIMP development. A slightly outdated but still valid document discussing this topic can be found at http://developer.gimp.org/gimp-future > And to think that only a handful of months ago I wondered why the > excitment and number of gimp developers had died down so much... I had the impression this situation lately started to improve. Hopefully I haven't destroyed this trend by starting this thread. 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, Nathan C Summers wrote: > This may be a bug when used with unsigned numbers, but it certainly is > a valid and acceptable approach to go out of range with signed > numbers, which is what the program was using. Since the MAX macro > returns a 1 for all nonpositive numbers, it's not a problem. > If this weren't ok, why would we need the CLAMP macro? Always staying > in range can be a difficult and inefficient way of doing things. Right. > Changing signed to unsigned turns this nonbug into a bug. While it is > possible to rewrite this particular example by using a (hypothetical) > function saturated_subtraction(width, 15, 1) which has some nastiness > to ensure that width never goes negative, I personally find that to > be a less readable way of doing things. (although such a function > could be useful at times.) It could also be implemented as a macro, > although the amount of nastiness required for doing that is scarcely > imaginable. 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. >> 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. > How do you propose to do so? Add a "valid" bit to each structure for > each varible used in this way? There go any space savings you might > have had. No, this is not my idea but your claim with the space savings is not quite correct. In most cases the bytes in a bitfield are not completely occupied and as such adding another bit wouldn't make a difference for the total size. 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 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. > I personally find nothing wrong with signifying an invalid > value with an invalid value. 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. > This is a very accepted practice in C, and even in places and > languages that try avoid such usage end up doing it with pointers. > I've only seen one language that doesn't have an equivalent to > "NULL", the pointer that isn't a pointer -- the invalid pointer. I > fail to see why using NULL to signify an invalid value for a pointer > is any different than using negative numbers to signify an invalid > "unsigned" number. 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. -- 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, Michael Natterer wrote:
> 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.
It is a bug in the code, if a value that is supposed to be positive
goes bezerk and it's just hidden by the fact that the value is in a
signed type then you'll be in trouble somewhere. Using unsigned values
is not only right but also demonstrates where the problems are because
the compiler will do additional checks and if such a situation in a loop
occurs it'll be easy to track down because of the absurdity instead of
silently producing wrong results or leading to mysterious crashes later.
> 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?
Is a preview supposed to be of size 0? If yes we might want to take the
pointer of size return an gboolean, if no there is no problem at all.
But -1? Why not -2?
>> 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.
Yes, yes. "Doctor, doctor, it hurts when I do this...". If this really
is an issue to you I can write you a script that checks that all given
parameters of type gboolean are either TRUE or FALSE. I still hope that
at some point we'll globally have the type bool in all C compilers so
this won't be an issue at all.
> It makes sense and kindof *must* be used for unsigned bitfields. See
> above.
We could also introduce an assertion here if you're scared that our own
code (after all this is still within GIMP) will pass arbitrary values.
> 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.
I'm not at all pissed of that you spend a lot of time with backing me
out when it would have been a "patch -p0 -R 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
