Bug 53840 – nrwt multiprocessing: conclude refactoring of dump_render_tree_thread and single_test_runner

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

[Dirk Pranke]

nrwt multiprocessing: conclude refactoring of dump_render_tree_thread and single_test_runner

[Dirk Pranke]

Created attachment 81342 [details]
Patch

[Ojan Vafai]

Comment on attachment 81342 [details]
Patch

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

> Tools/ChangeLog:23
> +        new-run-webkit-tests: move remaining common logic out of
> +        dump_render_tree_thread into single_test_runner so that we can
> +        reuse it in the new multiprocessing worker class as well.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=53840
> +
> +        * Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:
> +        * Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:
> + 
> [phone]  Dirk Pranke  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        new-run-webkit-tests: refactor common logic out of
> +        dump_render_tree_thread into single_test_runner so that we can
> +        reuse it in the new multiprocessing worker class as well.
> +
> +        * Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:
> +        * Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:

duplicate entry

> Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:205
> +                    self._http_lock_wait_begin = time.time()

Should you move http_lock_wait_* as well? Seems like it would make sense to keep it with the logic that starts/stops the server.

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 81342 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81342&action=review
> 
> > Tools/ChangeLog:23
> > +        new-run-webkit-tests: move remaining common logic out of
> > +        dump_render_tree_thread into single_test_runner so that we can
> > +        reuse it in the new multiprocessing worker class as well.
> > +
> > +        https://bugs.webkit.org/show_bug.cgi?id=53840
> > +
> > +        * Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:
> > +        * Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:
> > + 
> > [phone]  Dirk Pranke  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        new-run-webkit-tests: refactor common logic out of
> > +        dump_render_tree_thread into single_test_runner so that we can
> > +        reuse it in the new multiprocessing worker class as well.
> > +
> > +        * Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:
> > +        * Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:
> 
> duplicate entry
> 

Will fix.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:205
> > +                    self._http_lock_wait_begin = time.time()
> 
> Should you move http_lock_wait_* as well? Seems like it would make sense to keep it with the logic that starts/stops the server.

Good question. I think the answer is no. The reason is non-obvious. the TestRunner thread reads the nextTimeout() value from the TestShellThread, and that timeout value includes the time being spent acquiring the lock (which could take an unbounded amount of time if some other NRWT is currently holding the lock). In order to account for this, the TestShellThread needs to offset the nextTimeout by both the being and end values. Which means both variables need to be exposed to TestShellThread.

On the other hand, the Worker code triggers timeouts differently, and doesn't need to account for the time spent getting the lock at all. So, it doesn't need timing code and hence if the code was pushed into single_test_runner it would go unused. 

So, only one caller needs the code, and the code is most easily expressed as before/after logic anyway => code stays in TestShellThread.

Make sense?

[Dirk Pranke]

Created attachment 81591 [details]
fix baseline for merge

[Dirk Pranke]

Created attachment 81597 [details]
fix baseline for merge

[Dirk Pranke]

Okay, I think the bad baseline for the merge made this patch more confusing than it needed to be. I've redone it, and it should be clearer. The one main change - in TestShellThread._run(), most of the code gets wrapped in a try...finally block to make sure we stop the servers and release the file lock if necessary.

[Tony Chang]

Comment on attachment 81597 [details]
fix baseline for merge

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

Aside from the huge try/finally, seems fine.

> Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:193
> +        try:
> +            while True:
> +                if self._canceled:
> +                    _log.debug('Testing cancelled')

Having a huge try/finally block is hard to follow.  I would prefer splitting the code into a separate function and having the try/finally be around just the function call (still hard to follow, but makes the whitespace/indention easier to see).

Alternately, the try/finally part doesn't seem related to the refactoring.  Maybe it should be in a separate patch?

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 81597 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81597&action=review
> 
> Aside from the huge try/finally, seems fine.
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:193
> > +        try:
> > +            while True:
> > +                if self._canceled:
> > +                    _log.debug('Testing cancelled')
> 
> Having a huge try/finally block is hard to follow.  I would prefer splitting the code into a separate function and having the try/finally be around just the function call (still hard to follow, but makes the whitespace/indention easier to see).
> 
> Alternately, the try/finally part doesn't seem related to the refactoring.  Maybe it should be in a separate patch?

Ok, I'll defer that to a separate patch.

[Dirk Pranke]

Created attachment 81719 [details]
remove giant try/finally

[Dirk Pranke]

Created attachment 81720 [details]
remove giant try/finally, take two

[Dirk Pranke]

Committed r77998: <http://trac.webkit.org/changeset/77998>

[Hayato Ito]

I am sorry that I should have mentioned my intention of introducing single_test_runner.
My intention of introducing single_test_runner module is that the module should not include any code related with thread management. I'd like to separate concerns.

I meant that client of this module should only use single_test_runner.run_test() function. Any thread management should be done in caller side of this function, dump_render_tree. I meant that SingleTestRunner class is only used internally in this module and don't have to take care of state of driver. This module's only responsibility is to provide single_test_runner.run_test() function, which is used as if 'stateless' function, and should not have any side effect, except for writing test result to result directory.

The patch itself is okay for me. Let me continue thinking.

[Dirk Pranke]

(In reply to comment #12)
> I am sorry that I should have mentioned my intention of introducing single_test_runner.
> My intention of introducing single_test_runner module is that the module should not include any code related with thread management. I'd like to separate concerns.
> 
> I meant that client of this module should only use single_test_runner.run_test() function. Any thread management should be done in caller side of this function, dump_render_tree. I meant that SingleTestRunner class is only used internally in this module and don't have to take care of state of driver. This module's only responsibility is to provide single_test_runner.run_test() function, which is used as if 'stateless' function, and should not have any side effect, except for writing test result to result directory.
> 
> The patch itself is okay for me. Let me continue thinking.

Hi Ito-san,

I think your intended design is good. At the moment I'm stuck in mid-merge and trying to reduce the amount of code duplicated between classes, and so I stuffed some of the logic into SingleTestRunner that shouldn't necessarily have been there. I think I need to create a separate base class / mixin for both TestShellThread and the Worker class. I'll add that in an upcoming patch, so hopefully my abuse of your design won't last long :)

[Dirk Pranke]

(In reply to comment #13)
> Hi Ito-san,
> 
> I think your intended design is good. At the moment I'm stuck in mid-merge and trying to reduce the amount of code duplicated between classes, and so I stuffed some of the logic into SingleTestRunner that shouldn't necessarily have been there. I think I need to create a separate base class / mixin for both TestShellThread and the Worker class. I'll add that in an upcoming patch, so hopefully my abuse of your design won't last long :)

Whoops, hit reply too soon. I should have mentioned that I'm not sure that I necessarily agree with single_test_runner needing to be a only have a single function, however, or that the driver should be passed to it. Why is that better than having the SingleTestRunner create and own the driver?