Re: [Gimp-developer] [PATCH 1/4] Tile caching performance patches
On Wed, Jun 3, 2009 at 3:12 AM, Sven Neumann s...@gimp.org wrote: There should not be any trailing whitespace in the GIMP source code. We have several times trimmed away all trailing whitespace and committed these cleanups. If new trailing whitespace sneaked in, then we should probably do that again. You're right and I was vaguely astounded to see that. 'Show whitespace' seems like the right solution, not forcing anything. [BTW, did I manage to avoid any slipups in the new patches?] Monty ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
[Gimp-developer] resubmitted patches: tile-cache performance improvements
Updated patches attached, including all requested coding style changes. Unless, of course, I missed any. Monty 0001-Minor-change-to-TILE_DATA_POINTER-that-restricts-TIL.patch Description: Binary data 0002-Add-additional-profiling-to-tile-usage-in-order-to-a.patch Description: Binary data 0003-Replace-two-list-flush-clean-first-cache-strategy-wi.patch Description: Binary data 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
[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] [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 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
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
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
[Gimp-developer] [PATCH] fix to batch console support
The batch mode console is using stdio but never flushing its output after fwrite()s. This still usually works by accident when using batch mode from a terminal, but it means scripts generally won't work as responses to requests never get flushed through the return pipe. Apologies for this not being in git format-patch format, it was a spot test to get other unit tests working and trivial enough it doesn't seem worth the extra trouble. Cheers, Monty diff --git a/plug-ins/script-fu/scheme-wrapper.c b/plug-ins/script-fu/scheme-wrapper.c index 9212801..fbcf81b 100644 --- a/plug-ins/script-fu/scheme-wrapper.c +++ b/plug-ins/script-fu/scheme-wrapper.c @@ -329,6 +329,7 @@ ts_stdout_output_func (TsOutputType type, if (len 0) len = strlen (string); fprintf (stdout, %.*s, len, string); + fflush (stdout); } void ___ Gimp-developer mailing list Gimp-developer@lists.XCF.Berkeley.EDU https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
