[Gimp-developer] [PATCH 0/4] Tile caching performance patches
While working on my resampler, I noticed the tile cache often got itself into near-deadlock situations when it was actually working under moderate cache pressure. I have a series of four tile cache performance patches following this mail; one is a simple one-liner that removes a few integer divisions, the second adds a decent amount of profiling collection and output to the tile cache, the third fixes the tile cache strategy and the fourth addresses bugs and tuning problems with the idle swapper. Along with these patches, I've also written automated test/profiling scripts. Detailed description of the patches, test scripts and some profilng results can be found at http://web.mit.edu/xiphmont/Public/gimp-fu/gimp-cache.html The patches are against the gnome master, but I believe they can be [and should be] applied to 2.6 as well. Cheers, Monty ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
[Gimp-developer] [PATCH 1/4] Tile caching performance patches
Patch attached [to avoid any chance of gmail mangling lines] Detailed patch description at: http://web.mit.edu/xiphmont/Public/gimp-fu/gimp-cache.html Monty 0001-Minor-change-to-TILE_DATA_POINTER-that-restricts-TIL.patch Description: Binary data ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
[Gimp-developer] [PATCH 2/4] Tile caching performance patches
Patch attached [to avoid any chance of gmail mangling lines] Detailed patch description at: http://web.mit.edu/xiphmont/Public/gimp-fu/gimp-cache.html Monty 0002-Add-additional-profiling-to-tile-usage-in-order-to-a.patch Description: Binary data ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
[Gimp-developer] [PATCH 3/4] Tile caching performance patches
Patch attached [to avoid any chance of gmail mangling lines] Detailed patch description at: http://web.mit.edu/xiphmont/Public/gimp-fu/gimp-cache.html Monty 0003-Replace-two-list-flush-clean-first-cache-strategy-wi.patch Description: Binary data ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
[Gimp-developer] [PATCH 4/4] Tile caching performance patches
Patch attached [to avoid any chance of gmail mangling lines] Detailed patch description at: http://web.mit.edu/xiphmont/Public/gimp-fu/gimp-cache.html Monty 0004-Correct-startup-flaw-in-idle-swapper-start-Don-t-wat.patch Description: Binary data ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] Hacking gimp
Hi, Thanks! That's exactly what I needed. - Jordan On Mon, Jun 1, 2009 at 11:44 PM, Martin Nordholts ense...@gmail.com wrote: Jordan Stinson wrote: Hi, I'm attempting to add new files to the app/widgets directory. I've added a .h and a .c file and the compiler tries to compile it. I seem to be able to compile code in these files however, when I try to include gimp.h, I get an error in gimpobject.h line 35 saying something like, expected specifier-qualifier-list before 'GObject' I'm not sure what I'm doing wrong and I can't seem to find anything on the gimp developer site or in any of the archives for the mailing list. Is there a prescribed way to add files? If so, where can I find this information? Hi! This doesn't sound like an add-file problem but an include-problem. More specifically, it seems that you include gimp.h before the GObject header is pulled in. Did you read the GIMP include policy in devel-docs/includes.txt? You can see how other files in the app/widgets dir include gimp.h, for example gimpcolordialog.c. That file, like most files in that dir, includes the GObject header indirectly by pulling in the GTK+ header: #include gtk/gtk.h and then after widgets-types.h comes gimp.h: #include core/gimp.h Let us know if you need further assistance. Best regards, Martin ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
[Gimp-developer] Hacking GIMP - Gimp top level menu
Hi, I was wondering how the top level menu for GIMP is created (The one with these items: File, Edit, Select, View, Image, etc. ). I want to add a menu item to this menu and I'm kind of lost as to where to start. If someone could tell me where to hunt around in the code for this and give me an explanation of how it's done, that would be great! Even better, is there some documentation online about this kind of thing? Thanks in advance, Jordan ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 1/4] Tile caching performance patches
Hi, first of all thanks a lot for providing these patches. I definitely want to get them merged as soon as possible. But there are a few minor issues that should be discussed first. So let me start by commenting on your first patch: On Tue, 2009-06-02 at 04:11 -0400, Christopher Montgomery wrote: #define TILE_DATA_POINTER(tile,x,y) \ ((tile)-data + \ - (((y) % TILE_HEIGHT) * (tile)-ewidth + ((x) % TILE_WIDTH)) * (tile)-bpp) - + (((y) (TILE_HEIGHT-1)) * (tile)-ewidth + ((x) (TILE_WIDTH-1))) * (tile)-bpp) As far as I know pretty much any compiler out there should be able to replace a modulo by a power-of-2 constant by the bit-wise AND operation without us explicitly doing so (see also http://en.wikipedia.org/wiki/Modulo_operation#Performance_issues). So for the benefit of readable code I suggest that we keep the code as it is. Sven ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 2/4] Tile caching performance patches
Hi, On Tue, 2009-06-02 at 04:12 -0400, Christopher Montgomery wrote: +#ifdef TILE_PROFILING +#include sys/time.h If we use GTimeVal instead of struct timeval, we can avoid this include (and a possible portability problem). +#ifdef TILE_PROFILING + if ((cur_cache_size + tile-size) max_cache_size){ + struct timeval now; + struct timeval later; + gettimeofday(now,NULL); Please use GTimeVal and g_get_current_time() here. + if(tile_total_interactive_usec 0){ + tile_total_interactive_usec += 100; + tile_total_interactive_sec--; + } These lines don't adhere to the coding style guidelines. Please add a space after the if and move the opening curly bracket to the next line. There are other places in your patches that need similar fixes. Other than these nit-picking style issues, the patch looks good to me. Nice work! Sven ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 1/4] Tile caching performance patches
On Tue, Jun 2, 2009 at 4:46 PM, Sven Neumann s...@gimp.org wrote: Hi, first of all thanks a lot for providing these patches. I definitely want to get them merged as soon as possible. But there are a few minor issues that should be discussed first. So let me start by commenting on your first patch: On Tue, 2009-06-02 at 04:11 -0400, Christopher Montgomery wrote: #define TILE_DATA_POINTER(tile,x,y) \ ((tile)-data + \ - (((y) % TILE_HEIGHT) * (tile)-ewidth + ((x) % TILE_WIDTH)) * (tile)-bpp) - + (((y) (TILE_HEIGHT-1)) * (tile)-ewidth + ((x) (TILE_WIDTH-1))) * (tile)-bpp) As far as I know pretty much any compiler out there should be able to replace a modulo by a power-of-2 constant by the bit-wise AND operation without us explicitly doing so (see also http://en.wikipedia.org/wiki/Modulo_operation#Performance_issues). So for the benefit of readable code I suggest that we keep the code as it is. Interesting. I got a noticable and repeatable performance benefit. Which is not to say I haven't somehow mismeasured it. I agree the modulo is more readable. ...perhaps the difference is the difference of (x) or (y) possibly being negative and additional conformance-related assembly getting generated? I suppose there's no reason to speculate, I'll go read the assembly gcc generates and that will answer everything, at least for me. Monty ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 3/4] Tile caching performance patches
Hi, a few more coding style despite the ones I already pointed out: + if(!tile)return FALSE; Please write this as if (! tile) return FALSE; + if(PENDING_WRITE(t)) +acc+=t-size; + t=t-next; Please insert an empty line like this: if (PENDING_WRITE (t)) acc += t-size; t = t-next; Other than these style issues, the patch looks good. In particular your benchmarking data looks promising. Nice work! Sven ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 2/4] Tile caching performance patches
+#ifdef TILE_PROFILING +#include sys/time.h If we use GTimeVal instead of struct timeval, we can avoid this include (and a possible portability problem). Agreed. These lines don't adhere to the coding style guidelines. Please add a space after the if and move the opening curly bracket to the next line. There are other places in your patches that need similar fixes. I'll make that change too and resubmit. Monty ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 4/4] Tile caching performance patches
Hi, On Tue, 2009-06-02 at 04:13 -0400, Christopher Montgomery wrote: -#define IDLE_SWAPPER_TIMEOUT 250 - +#define IDLE_SWAPPER_START 1000 +#define IDLE_SWAPPER_INTERVAL 20 +#define IDLE_SWAPPER_TILES_PER 10 Should that constant perhaps better be called IDLE_SWAPPER_TILES_PER_RUN ? Sven ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 4/4] Tile caching performance patches
On Tue, Jun 2, 2009 at 5:01 PM, Sven Neumann s...@gimp.org wrote: Hi, On Tue, 2009-06-02 at 04:13 -0400, Christopher Montgomery wrote: -#define IDLE_SWAPPER_TIMEOUT 250 - +#define IDLE_SWAPPER_START 1000 +#define IDLE_SWAPPER_INTERVAL 20 +#define IDLE_SWAPPER_TILES_PER 10 Should that constant perhaps better be called IDLE_SWAPPER_TILES_PER_RUN ? Or PER_INTERVAL, sure. Monty ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 1/4] Tile caching performance patches
Hi, On Tue, 2009-06-02 at 16:56 -0400, Christopher Montgomery wrote: As far as I know pretty much any compiler out there should be able to replace a modulo by a power-of-2 constant by the bit-wise AND operation without us explicitly doing so (see also http://en.wikipedia.org/wiki/Modulo_operation#Performance_issues). So for the benefit of readable code I suggest that we keep the code as it is. Interesting. I got a noticable and repeatable performance benefit. Which is not to say I haven't somehow mismeasured it. I agree the modulo is more readable. ...perhaps the difference is the difference of (x) or (y) possibly being negative and additional conformance-related assembly getting generated? I suppose there's no reason to speculate, I'll go read the assembly gcc generates and that will answer everything, at least for me. I might very well be wrong here. If there's indeed a difference in the generated assembly and a noticeable performance benefit, than let's use the optimized macro. But perhaps we can add a short comment there explaining that ((y) (TILE_HEIGHT-1)) is equivalent to ((y) % TILE_HEIGHT). Not everyone reading this code will be aware of this immediately. Sven ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
[Gimp-developer] Gimp Distro?
I could not figure out if this is the right list of if the topic has come up before. Is there a Linux distro designed to be a training/learning/use/marketing distro for the Gimp? ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 1/4] Tile caching performance patches
I'd like to mention also that there are also some minor problems with whitespace i...@gbubuntu:~/st/gimp2/gimp$ git-am /tmp/0002*.patch Applying Add additional profiling to tile usage in order to analyze efficiency and behavior of the tile cache. Profiling includes run-time indication of idle swapper activity. .dotest/patch:193: trailing whitespace. guint zorched : 1;/* was the tile flushed due to cache pressure .dotest/patch:255: trailing whitespace. #endif .dotest/patch:304: trailing whitespace. #ifdef TILE_PROFILING .dotest/patch:318: trailing whitespace. .dotest/patch:319: trailing whitespace. #ifdef TILE_PROFILING warning: squelched 12 whitespace errors warning: 17 lines add whitespace errors. i...@gbubuntu:~/st/gimp2/gimp$ git-am /tmp/0003*.patch Applying Replace two list 'flush clean first' cache strategy with an LRu strategy. Although the clean-first strategy gives fast light-load performance, it also degrades catastrophically under moderate cache pressure. LRU is not as efficient under light load, but degrades more gracefully under moderate and heavy load. .dotest/patch:148: trailing whitespace. .dotest/patch:191: trailing whitespace. .dotest/patch:196: trailing whitespace. .dotest/patch:202: trailing whitespace. .dotest/patch:205: trailing whitespace. warning: squelched 8 whitespace errors warning: 13 lines add whitespace errors. i...@gbubuntu:~/st/gimp2/gimp$ git-am /tmp/0004*.patch Applying Correct startup flaw in idle swapper start: Don't watch only UI idling, but also watch that the cache itself is idle. Previously it would start during transforms and long pyramid rendering ops and toss writes and large seeks into the tile cache while it was potentially under heavy pressure. .dotest/patch:149: trailing whitespace. .dotest/patch:157: trailing whitespace. .dotest/patch:158: trailing whitespace. if(count=IDLE_SWAPPER_TILES_PER) .dotest/patch:186: trailing whitespace. .dotest/patch:194: trailing whitespace. warning: squelched 1 whitespace error warning: 6 lines add whitespace errors. (patch 0001 applies with no problems.) ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 1/4] Tile caching performance patches
For about a month I'd turned on emacs's trailing whitespace autotrim and it was a cure worse than the disease. How shall I kill my own whitespace without generating patches 4x larger than necessary due to others' trailing whitespace? Monty ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 1/4] Tile caching performance patches
On Tue, Jun 2, 2009 at 10:15 PM, Christopher Montgomery xiphm...@gmail.com wrote: For about a month I'd turned on emacs's trailing whitespace autotrim and it was a cure worse than the disease. How shall I kill my own whitespace without generating patches 4x larger than necessary due to others' trailing whitespace? [best I've struck on is manually running M-x delete-trailing-whitespace on each file, followed by git format-patch -b. Surely for something that can be done completely mechanically, there's some better way...] Monty ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 1/4] Tile caching performance patches
In fact, with -O2, gcc is generating more complex assembly for % than , though not an integer division. Assembly generated for version using : tile_data_pointer: .LFB29: movzwl 8(%rdi), %eax andl$63, %edx andl$63, %esi imull %eax, %edx movzbl 7(%rdi), %eax addl%esi, %edx imull %eax, %edx movslq %edx,%rax addq24(%rdi), %rax ret assembly generated for version using %: tile_data_pointer: .LFB29: movl%edx, %eax sarl$31, %eax shrl$26, %eax addl%eax, %edx andl$63, %edx subl%eax, %edx movzwl 8(%rdi), %eax imull %eax, %edx movl%esi, %eax sarl$31, %eax shrl$26, %eax addl%eax, %esi andl$63, %esi subl%eax, %esi movzbl 7(%rdi), %eax addl%esi, %edx imull %eax, %edx movslq %edx,%rax addq24(%rdi), %rax ret On Tue, Jun 2, 2009 at 5:09 PM, Sven Neumann s...@gimp.org wrote: Hi, On Tue, 2009-06-02 at 16:56 -0400, Christopher Montgomery wrote: As far as I know pretty much any compiler out there should be able to replace a modulo by a power-of-2 constant by the bit-wise AND operation without us explicitly doing so (see also http://en.wikipedia.org/wiki/Modulo_operation#Performance_issues). So for the benefit of readable code I suggest that we keep the code as it is. Interesting. I got a noticable and repeatable performance benefit. Which is not to say I haven't somehow mismeasured it. I agree the modulo is more readable. ...perhaps the difference is the difference of (x) or (y) possibly being negative and additional conformance-related assembly getting generated? I suppose there's no reason to speculate, I'll go read the assembly gcc generates and that will answer everything, at least for me. I might very well be wrong here. If there's indeed a difference in the generated assembly and a noticeable performance benefit, than let's use the optimized macro. But perhaps we can add a short comment there explaining that ((y) (TILE_HEIGHT-1)) is equivalent to ((y) % TILE_HEIGHT). Not everyone reading this code will be aware of this immediately. Sven ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
Re: [Gimp-developer] [PATCH 1/4] Tile caching performance patches
Christopher Montgomery wrote: For about a month I'd turned on emacs's trailing whitespace autotrim and it was a cure worse than the disease. How shall I kill my own whitespace without generating patches 4x larger than necessary due to others' trailing whitespace? Caring too much about trailing whitespace is IMO a good way to waste time. Just make sure to not commit trailing whitespace, and remove trailing whitespace on lines you change, and that's fine. Having an editor automatically remove trailing whitespace on save just breaks git-blame and pollutes diffs. / Martin ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer