Bug 36899 - Make DumpRenderTree parallelizable (fix local database concurent access)
Summary: Make DumpRenderTree parallelizable (fix local database concurent access)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 33153
  Show dependency treegraph
 
Reported: 2010-03-31 12:43 PDT by Csaba Osztrogonác
Modified: 2010-04-09 04:57 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch (3.15 KB, patch)
2010-03-31 13:08 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
proposed patch (3.16 KB, patch)
2010-03-31 23:09 PDT, Csaba Osztrogonác
eric: review-
Details | Formatted Diff | Diff
updated patch (3.19 KB, patch)
2010-04-07 03:44 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (3.49 KB, patch)
2010-04-09 00:12 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (3.41 KB, patch)
2010-04-09 00:16 PDT, Csaba Osztrogonác
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2010-03-31 12:43:58 PDT
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.
Comment 1 Csaba Osztrogonác 2010-03-31 13:08:09 PDT
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.
Comment 2 David Levin 2010-03-31 14:12:29 PDT
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.
Comment 3 Csaba Osztrogonác 2010-03-31 14:49:54 PDT
(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
Comment 4 Darin Adler 2010-03-31 17:38:13 PDT
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.
Comment 5 Csaba Osztrogonác 2010-03-31 23:09:31 PDT
Created attachment 52261 [details]
proposed patch

Mac code is fixed by the advice of Darin.
Comment 6 Csaba Osztrogonác 2010-04-06 04:56:50 PDT
Anybody for review, please?
Comment 7 Eric Seidel (no email) 2010-04-06 20:57:51 PDT
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?
Comment 8 Csaba Osztrogonác 2010-04-07 02:50:27 PDT
(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. :))
Comment 9 Csaba Osztrogonác 2010-04-07 03:44:15 PDT
Created attachment 52726 [details]
updated patch

use local variable instead of calling getenv many times
Comment 10 Csaba Osztrogonác 2010-04-08 23:17:56 PDT
(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)
Comment 11 Eric Seidel (no email) 2010-04-08 23:31:48 PDT
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.
Comment 12 Csaba Osztrogonác 2010-04-09 00:12:49 PDT
Created attachment 52942 [details]
Patch
Comment 13 Csaba Osztrogonác 2010-04-09 00:16:32 PDT
Created attachment 52943 [details]
Patch
Comment 14 Eric Seidel (no email) 2010-04-09 00:17:36 PDT
Comment on attachment 52943 [details]
Patch

OK.  I look forward to hearing good things from the Qt bot.
Comment 15 Csaba Osztrogonác 2010-04-09 04:57:38 PDT
(In reply to comment #13)
> Created an attachment (id=52943) [details]
> Patch

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