Bug 48820 – nrwt - checkpoint multiprocess work

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Dirk Pranke]

nrwt - checkpoint multiprocess work

[Dirk Pranke]

Created attachment 72633 [details]
Patch

[Dirk Pranke]

checkpointing work in progress. The code seems to work and not wedge any more. But, it needs a lot more testing and cleanup, and then it needs to be broken into multiple chunks for review / landing.

[Dirk Pranke]

Created attachment 73184 [details]
Patch

[Dirk Pranke]

Created attachment 73185 [details]
Patch

[Dirk Pranke]

Created attachment 73186 [details]
merge to head (r71474)

[Dirk Pranke]

Created attachment 73301 [details]
merge from trunk@71580

[Dirk Pranke]

Created attachment 73722 [details]
cleanup, merge to tip of tree as of r71873

[Dirk Pranke]

Note that the code as it stands works fine on Mac and Linux (I think) if you want to apply it and start playing with it. We get a near-linear speed up on the tests up to at least 16 workers/processes (threads top out somewhere between 4 and 8, assuming things don't wedge) ...

[Eric Seidel (no email)]

Comment on attachment 73722 [details]
cleanup, merge to tip of tree as of r71873

View in context: https://bugs.webkit.org/attachment.cgi?id=73722&action=review

> WebKitTools/ChangeLog:10
> +        * Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:

So this isn't actually really a thread anymore?  I assume we'll fix the naming later.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:53
> +from webkitpy.layout_tests.test_types import test_type_base
> +from webkitpy.layout_tests.test_types import text_diff
> +from webkitpy.layout_tests.test_types import image_diff

Isn't this just one import line with commas?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:94
> +        _log.debug("%s Stacktrace for %s:\n%s" % (name, test_name, error))
> +        filename = os.path.join(options.results_directory, test_name)

Seems you're fixing path handling in this change as well as process/thread devisions which is slightly confusing.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:160
> +            self._driver.run_test(test_info.uri, test_info.timeout,
> +                                  test_info.image_checksum)

Seems we're almost just passing test_info, maybe that's the eventual goal.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:172
> +class Worker(object):

I'm assuming this object is in the parent process?  We should definitely separate parent vs. child classes in separate files.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:182
> +        self._container = container

It's probably more accurately a test_queue or input_queue?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:186
> +        self._name = 'worker-%d' % worker_number

Seems name() is just a function which uses worker_number.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:216
> +        if self._options.pixel_tests:
> +            png_path = os.path.join(self._options.results_directory,
> +                                    "png_result%s.png" %
> +                                    self._worker_number)
> +            test_args.png_path = png_path

It's always struck me as strange that CrDRT uses this file to pass the png data back to the parent instead of the parent just telling it where to write the final png.  But this is a long-standing design issue.  I guess other DRTs pass the data over the pipe, which has its own quirks.

[Dirk Pranke]

(In reply to comment #9)
> (From update of attachment 73722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73722&action=review
> 
> > WebKitTools/ChangeLog:10
> > +        * Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:
> 
> So this isn't actually really a thread anymore?  I assume we'll fix the naming later.
>

Yes (that was in the email).
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:53
> > +from webkitpy.layout_tests.test_types import test_type_base
> > +from webkitpy.layout_tests.test_types import text_diff
> > +from webkitpy.layout_tests.test_types import image_diff
> 
> Isn't this just one import line with commas?
> 

Could be. I usually import one package per line.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:94
> > +        _log.debug("%s Stacktrace for %s:\n%s" % (name, test_name, error))
> > +        filename = os.path.join(options.results_directory, test_name)
> 
> Seems you're fixing path handling in this change as well as process/thread devisions which is slightly confusing.
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:160
> > +            self._driver.run_test(test_info.uri, test_info.timeout,
> > +                                  test_info.image_checksum)
> 
> Seems we're almost just passing test_info, maybe that's the eventual goal.
> 

Yes, exactly. I told Ito-san that in a separate patch last night, and I think he might be making that change.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:172
> > +class Worker(object):
> 
> I'm assuming this object is in the parent process?  We should definitely separate parent vs. child classes in separate files.
>

No.  Worker objects are owned by the (perhaps not yet well named) WorkerThunk object in router.py. WorkerThunk is a base class subclassed by WorkerThread or WorkerProcess (also in router.py).

But, yes, they're in separate files.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:182
> > +        self._container = container
> 
> It's probably more accurately a test_queue or input_queue?
>

No, it's a container, in the JSEE sense, if that means anything to you. It's the WorkerThread/Process.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:186
> > +        self._name = 'worker-%d' % worker_number
> 
> Seems name() is just a function which uses worker_number.
>

Sure.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:216
> > +        if self._options.pixel_tests:
> > +            png_path = os.path.join(self._options.results_directory,
> > +                                    "png_result%s.png" %
> > +                                    self._worker_number)
> > +            test_args.png_path = png_path
> 
> It's always struck me as strange that CrDRT uses this file to pass the png data back to the parent instead of the parent just telling it where to write the final png.  But this is a long-standing design issue.  I guess other DRTs pass the data over the pipe, which has its own quirks.

Yes, I also want to change this so that DRT's abstraction is the one we use. This approach is lame because it makes us have to write the data we get from DRT into a file, which we then read back into memory so we can write it over a pipe to ImageDiff. It's design for Chromium's implementation, and we need to change it to the WebKit default.

See that same patch that Ito-san is working on for reftests.

[Dirk Pranke]

Created attachment 73937 [details]
change default # of child processes for test port back to 1, merge to ToT as of r72041

[Dirk Pranke]

Created attachment 74180 [details]
Patch

[Dirk Pranke]

Created attachment 74292 [details]
Patch

[Dirk Pranke]

Created attachment 74718 [details]
sync to r72641 + patch to bug 49779

[Dirk Pranke]

Created attachment 74816 [details]
sync to r72708

[Dirk Pranke]

Created attachment 75348 [details]
sync to r73080, clean up broker abstraction a bunch, clean up dump_render_tree_thread to track bug 50367

[Dirk Pranke]

Created attachment 75364 [details]
Patch - merge to r73080 plus the chain of bugs from 50367 .. 50381

[Dirk Pranke]

Created attachment 75867 [details]
Patch

[Dirk Pranke]

merged in latest patch changes from bugs 50557, 50375, and 50381, plus a few other minor changes/cleanup discovered in testing. Sync to ToT as of r73469.