Bug 36899 – Make DumpRenderTree parallelizable (fix local database concurent access)

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

Now we can't run more than one DumpRenderTree on one 
machine, because DRT processes use same temporary 
directory for local storage, icon database, etc.
(Or one DRT per user on Mac because of user temp.)

Running more DRT on one machine is useful if more then 
one people (or buildbot) work on a powerful multicore 
server, or if you would like to make run-webkit-test 
script parallel to decrease runtime of layouttesting.

[Csaba Osztrogonác]

Created attachment 52199 [details]
proposed patch

This patch tested and works on Qt-linux and on Mac-Leopard.

Now the temporary directory for local storage and icon db is 
~/.local on Qt, it will be $TMP/DumpRenderTree-XXXXXX with this patch.
On Leopard it was ~/Library/Application Support/DumpRenderTree
and it will be $TMP/DumpRenderTree-XXXXXX (here it was user temp)

This patch would solve many strange storage, workers crashes on Qt bots.

[David Levin]

I don't think the Mac OS X change is correct. There are a lot more places than this that use the home directory. It is better just to change the home directory for that platform rather than use this other environment variable.

[Csaba Osztrogonác]

(In reply to comment #2)
I think Mac DumpRenderTree only use home directory in this place.
I can't find other places, but it is possible there are.

As far as I know, now all database can be found here:
~/Library/Application Support/DumpRenderTree

I consider it isn't a good idea to change home directory,
I think it is an ugly hack. And I'm not sure if it works.

---
There are some other files in user temp, but
it is an other problem what we should fix:
(On Mac now it is /var/folders/zz/zz7dAsWSGUu8b-4hUGMKs++++TY/-Tmp-
On Qt-Linux there isn't user temp, there is only the global /tmp)
- access-control-preflight-headers-status
- appcache_counter
- appcache_manifest_counter
- network-simulator-state
- preflightCache.txt
- preflightCacheInvalidationByHeader.txt
- preflightCacheInvalidationByMethod.txt
- preflightCacheTimeout.txt
- resource-count

[Darin Adler]

Comment on attachment 52199 [details]
proposed patch

> +        return [NSString stringWithCString: getenv("DUMPRENDERTREE_TEMP")];

The correct function for this is:

    -[NSFileManager stringWithFileSystemRepresentation:length:]

The stringWithCString function will not work properly for non-ASCII characters.

[Csaba Osztrogonác]

Created attachment 52261 [details]
proposed patch

Mac code is fixed by the advice of Darin.

[Csaba Osztrogonác]

Anybody for review, please?

[Eric Seidel (no email)]

Comment on attachment 52261 [details]
proposed patch

I don't believe that this is sufficient to make Mac work, but it is probably better than what we have now.

Also, why call getenv so many times?  Why not just once and cache the result in a local variable?

Do we know if this makes the IconDatbase crashers on the Qt bot go away?

[Csaba Osztrogonác]

(In reply to comment #7)
> (From update of attachment 52261 [details])
> I don't believe that this is sufficient to make Mac work, but it is probably
> better than what we have now.
This patch isn't enough to make Mac work, but it is 
necessary to avoid concurent database file accessses.

We have ideas to fix other problems mentioned here and 
in the master bug. But I don't want to upload a very big 
patch for review. I prefer one patch for one separated fix.
 
> Also, why call getenv so many times?  Why not just once and cache the result in
> a local variable?
You're right, it isn't so nice, I'll fix. New patch is coming soon.
 
> Do we know if this makes the IconDatbase crashers on the Qt bot go away?
I'm not sure enough, but I hope yes. I didn't want to apply this patch
for testing on the official bot before landing, because I'm afraid of svn
conflict and following fresh build. :))

[Csaba Osztrogonác]

Created attachment 52726 [details]
updated patch

use local variable instead of calling getenv many times

[Csaba Osztrogonác]

(In reply to comment #9)
> Created an attachment (id=52726) [details]
> updated patch
> use local variable instead of calling getenv many times

One more reaseon why this patch is useful:
https://bugs.webkit.org/show_bug.cgi?id=37279
(We really need it to avoid false positive alarms)

[Eric Seidel (no email)]

Comment on attachment 52726 [details]
updated patch

Stylistically this is wrong.  variable names are wrong, spacing is wrong.  You changed only one caller of getenv.

This is an incomplete hack.  It's a step in the right direction, but not a full fix.

[Csaba Osztrogonác]

Created attachment 52942 [details]
Patch

[Csaba Osztrogonác]

Created attachment 52943 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 52943 [details]
Patch

OK.  I look forward to hearing good things from the Qt bot.

[Csaba Osztrogonác]

(In reply to comment #13)
> Created an attachment (id=52943) [details]
> Patch

Landed in: http://trac.webkit.org/changeset/57328