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

Hi,

On Tue, 2009-06-02 at 04:12 -0400, Christopher Montgomery wrote:

+#ifdef TILE_PROFILING
+#include

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 += 1000000; + 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

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

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

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

I'd like to mention also that there are also some minor problems with whitespace

" ion@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. ion@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. ion@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.)

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

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

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