Re: [Gimp-developer] [PATCH 1/4] Tile caching performance patches
Hi, On Wed, 2009-06-03 at 11:41 +0930, David Gowers wrote: I'd like to mention also that there are also some minor problems with whitespace Right. I suggest to add the following lines to your .emacs file: (setq c-mode-common-hook '(lambda () (setq indent-tabs-mode nil) (setq show-trailing-whitespace true))) 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
Hi, On Tue, 2009-06-02 at 22:15 -0400, 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? 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. Now that git warns about trailing whitespace, we have a good chance to finally end this disease. I will commit another global whitespace cleanup later today. 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 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] [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
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 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 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
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
