Bug 47220 – new-run-webkit-tests - enable cygwin support for chromium win

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

[Dirk Pranke]

new-run-webkit-tests - enable cygwin support for chromium win

[Dirk Pranke]

Created attachment 69852 [details]
Patch

[Dirk Pranke]

Comment on attachment 69852 [details]
Patch

work in progress ... not ready for review

[Dirk Pranke]

Created attachment 69870 [details]
Patch

[Dirk Pranke]

Comment on attachment 69870 [details]
Patch

whoops ... definitely not intended for review.

[Dirk Pranke]

Created attachment 70387 [details]
Patch

[Dirk Pranke]

right, this patch should actually be the "right" way to do it.

Note that running under cygwin appears to be substantially slower than running under win32, so I wouldn't recommend it.

[Eric Seidel (no email)]

Comment on attachment 70387 [details]
Patch

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

Please see nits above, but generally looks fine.

> WebKitTools/Scripts/webkitpy/common/system/path.py:41
> +def cygpath(path, executive):

I'm generally scared of free functions, but it certainly fits the pattern of the file, and I'm not sure I have a better option to suggest.

> WebKitTools/Scripts/webkitpy/common/system/path.py:45
> +    # FIXME: this may not be correct in every situation, but forking
> +    # cygpath is very slow and seems to contribute to deadlocking issues
> +    # with threads.

The deadlock issue is a risk any time we fork a process from a non-main thread.  If I remember correctly, it's a race regarding the list of open pipes in subprocess?  So yes, it contributes, but we should be specific as to why or just not mention it at all, like:
FIXME: Forking to execute cygpath is slow, so we use a short-cut for some paths.

> WebKitTools/Scripts/webkitpy/common/system/path.py:50
> +    return executive.run_command(['cygpath', '-wa', path],

I wonder how often we execute this.  This might explain part of the slowness you see for CYGWIN NRWT.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:151
> +            cmd = [executable, '--diff',
> +                   self._convert_path(expected_filename),
> +                   self._convert_path(actual_filename),
> +                   self._convert_path(diff_filename)]

This seems like were likely to get this wrong. :)

We could add some code to executive which looked for / in argument names and ran the cygpath stuff automatically unless we passed a no_automatic_cygpath=false or something.  Maybe that's too magical?

Also (and I'm not sure this is worth it, but I should mention).  It might make sense to stick them all in a tuple and use map(self._convert_path, tuple) depending.

[Adam Roben (:aroben)]

Comment on attachment 70387 [details]
Patch

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

>> WebKitTools/Scripts/webkitpy/common/system/path.py:50
>> +    return executive.run_command(['cygpath', '-wa', path],
> 
> I wonder how often we execute this.  This might explain part of the slowness you see for CYGWIN NRWT.

old-run-webkit-tests keeps a couple of persistent cygpath processes around, feeding them paths on stdin and reading the converted paths from stdout. When we implemented this it sped things up substantially. You can see the code we use here: <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/old-run-webkit-tests?rev=69385#L1694>.

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 70387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70387&action=review
> 
> Please see nits above, but generally looks fine.
> 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:41
> > +def cygpath(path, executive):
> 
> I'm generally scared of free functions, but it certainly fits the pattern of the file, and I'm not sure I have a better option to suggest.
>

Okay. I'll leave it as is.
 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:45
> > +    # FIXME: this may not be correct in every situation, but forking
> > +    # cygpath is very slow and seems to contribute to deadlocking issues
> > +    # with threads.
> 
> The deadlock issue is a risk any time we fork a process from a non-main thread.  If I remember correctly, it's a race regarding the list of open pipes in subprocess?  So yes, it contributes, but we should be specific as to why or just not mention it at all, like:
> FIXME: Forking to execute cygpath is slow, so we use a short-cut for some paths.
> 

Will update accordingly.

> > WebKitTools/Scripts/webkitpy/common/system/path.py:50
> > +    return executive.run_command(['cygpath', '-wa', path],
> 
> I wonder how often we execute this.  This might explain part of the slowness you see for CYGWIN NRWT.
> 

This gets called once per test to convert the test file name into a file:// URI; until I rearranged the code in the TestInfo constructor, this was getting called all up front, leading to a horrendous startup cost. 

So, it certainly makes things slow, but I think just generally speaking cygwin is substantially slower than native windows.

In addition, it's called whenever we need to do an ImageDiff (which is not very frequent).

> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:151
> > +            cmd = [executable, '--diff',
> > +                   self._convert_path(expected_filename),
> > +                   self._convert_path(actual_filename),
> > +                   self._convert_path(diff_filename)]
> 
> This seems like were likely to get this wrong. :)
> 
> We could add some code to executive which looked for / in argument names and ran the cygpath stuff automatically unless we passed a no_automatic_cygpath=false or something.  Maybe that's too magical?
> 

We only want to call this routine when we're launching a win32 process directly and need to convert filenames. I would be worried about trying to do anything "clever" that would try to guess at when the right time to do this is or how to do the tokenizing safely. 

I will add a comment explaining what's going on here, but apart from that I think it's best to leave the code as simple and straightforward.

> Also (and I'm not sure this is worth it, but I should mention).  It might make sense to stick them all in a tuple and use map(self._convert_path, tuple) depending.

I think using a map will just obscure things in this case.

[Dirk Pranke]

(In reply to comment #8)
> (From update of attachment 70387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70387&action=review
> 
> >> WebKitTools/Scripts/webkitpy/common/system/path.py:50
> >> +    return executive.run_command(['cygpath', '-wa', path],
> > 
> > I wonder how often we execute this.  This might explain part of the slowness you see for CYGWIN NRWT.
> 
> old-run-webkit-tests keeps a couple of persistent cygpath processes around, feeding them paths on stdin and reading the converted paths from stdout. When we implemented this it sped things up substantially. You can see the code we use here: <http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/old-run-webkit-tests?rev=69385#L1694>.

That's definitely an alternative. It's still gonna be slower than just doing the conversion directly in the python code, of course, but it's also safer.

It's unclear to me how important supporting this code path is, how unsafe it is, and how performance-sensitive it is. Chromium-win isn't normally run under cygwin, so this doesn't matter to the bots. 

For regular webkit win, I wonder if it's better to run the bots directly under Win32 as well, rather than trying to tune the cywin path?

[Dirk Pranke]

Created attachment 70569 [details]
Patch

[Peter Kasting]

(In reply to comment #10)
> (In reply to comment #8)
> > old-run-webkit-tests keeps a couple of persistent cygpath processes around, feeding them paths on stdin and reading the converted paths from stdout.
> 
> That's definitely an alternative. It's still gonna be slower than just doing the conversion directly in the python code, of course, but it's also safer.
> 
> It's unclear to me how important supporting this code path is, how unsafe it is, and how performance-sensitive it is. Chromium-win isn't normally run under cygwin, so this doesn't matter to the bots.

If it's just people like me running manually, then the safety factor of consistently using the cygpath pipe method seems more valuable than the "greatest possible raw speed" route.  But like you I'm uncertain whether that's the only audience.

[Dirk Pranke]

Created attachment 70581 [details]
Patch

[Dirk Pranke]

Committed r69638: <http://trac.webkit.org/changeset/69638>

[Dirk Pranke]

note that I fixed the changelog in the last version of the patch before I landed it.

[Adam Roben (:aroben)]

This seems to have broken Apple's Windows bots: http://build.webkit.org/builders/Windows%20Release%20%28Tests%29/builds/5098/steps/webkitpy-test/logs/stdio

[Dirk Pranke]

Created attachment 70702 [details]
Patch

[Dirk Pranke]

fix was rolled out, so reopening.

[Dirk Pranke]

Comment on attachment 70702 [details]
Patch

This is a revised version of the patch that implements a slow, conservative version, a fast version, and a version with a long-running cygpath process open.

It's a bit unclear which version is the best. The fast version is, well, fast, but may not work correctly in some cygwin configurations. The slow version is slow (test-webkitpy is almost 2x slower). The long-running cygpath version seems to work fine but python coughs up an error at the end of running test-webkitpy that I can't track down a reason for.

The error is something like:

Exception AttributeError: "'NoneType' object has no attribute 'error'" in <bound
 method Popen.__del__ of <subprocess.Popen object at 0x7e6e966c>> ignored

which makes me think something odd is happening during cleanup at process exit. This doesn't seem to happen except when running test-webkitpy, which changes the logging system in such a way as to make me think that this message is normally suppressed.

At any rate, It would be great if I can get some feedback on the different approaches. Adam, in particular it would be great if you can confirm that this patch works on your system and the benchmarking results are similar to mine.

[Eric Seidel (no email)]

Comment on attachment 70702 [details]
Patch

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

I'm generally a fan of this change.  I feel like there is a bit much going on for my little brain to follow it all, but that may still be OK.

Paths forward:
1. Use the magic of git (commit -i perhaps?) to split out the simple changes here and just get them rubberstamped and landed.
2. Land pretty-much as is and iterate from there.

If 1. sounds easy to you, lets do that.  Otherwise I could be convinced on #2.

If you want to go down #2, post a patch for review with the nits fixed and I'll r+ it.  Patches tend to be easier to understand on a second reading anyway.

Thanks!

> WebKitTools/Scripts/webkitpy/common/system/path.py:78
> +_cygpath_global_lock = None
> +_cygpath_global_object = None

Can we make these class members and have them still be global?  Then you can use self. or cls. to access them instead of global.

> WebKitTools/Scripts/webkitpy/common/system/path.py:81
> +def cygpath(path):

This method is a bit difficult to process at this size.  I wonder if there is something which coudl be broken out of this?

> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:210
> +if __name__ == '__main__':
> +    unittest.main()

We've slowly been removing these and just running individual modules using "test-webkitpy webkitpy.layout_tests.deduplicate_tests_unittest" for example.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:354
> +    def _convert_path(self, path):

Why is _convert_path a port thing?  Seems it's a FileSystem thing.  Maybe just a path() method in a common.filesystem.py?  Doesn't have to be in this patch, but just thinking about the proper long term place for this.  Especially with your previous tool.filesystem() proposal for handling mocking open, exists, etc. (Which Adam and I both are *huge* fans of.)

Oh, I guess we have a path module already.  Maybe the free-function goes there?  (I'm failing to come up with a good (ideally short) name...

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:381
> +            # See note above in diff_image() for why we need
> +            # _convert_path().

Did we just miss the 80c boundary? ;)

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:382
> +            driver_args.append("--pixel-tests=" +

I wonder if we should have a fancy Args class which knows how to handle things like this.  A special append_path, method for example.  Donno.  Just a thought for the future.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:182
> +                else:
> +                    return ''

WebKit has a policy of avoiding else after return (at least for our C++ code, and I think it should apply to python as well).

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:46
> +            os.unlink(self.guard_lock_file)

Seems this should have been _guard_lock_file (but obviously that's a separate change.

[Dirk Pranke]

(In reply to comment #20)
> (From update of attachment 70702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70702&action=review
> 
> I'm generally a fan of this change.  I feel like there is a bit much going on for my little brain to follow it all, but that may still be OK.
> 
> Paths forward:
> 1. Use the magic of git (commit -i perhaps?) to split out the simple changes here and just get them rubberstamped and landed.
> 2. Land pretty-much as is and iterate from there.
> 
> If 1. sounds easy to you, lets do that.  Otherwise I could be convinced on #2.
> 
> If you want to go down #2, post a patch for review with the nits fixed and I'll r+ it.  Patches tend to be easier to understand on a second reading anyway.
> 
> Thanks!
> 

As we discussed in person, this version of the patch was intended to demonstrate the alternatives; I didn't want to land it as-is. We figured out how to make the long_running variant run cleanly but using an atexit() method, so we'll go with that.

> > WebKitTools/Scripts/webkitpy/common/system/path.py:78
> > +_cygpath_global_lock = None
> > +_cygpath_global_object = None
> 
> Can we make these class members and have them still be global?  Then you can use self. or cls. to access them instead of global.
>

Done.
 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:81
> > +def cygpath(path):
> 
> This method is a bit difficult to process at this size.  I wonder if there is something which coudl be broken out of this?
>

It's much smaller now.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:210
> > +if __name__ == '__main__':
> > +    unittest.main()
> 
> We've slowly been removing these and just running individual modules using "test-webkitpy webkitpy.layout_tests.deduplicate_tests_unittest" for example.
>

I use this all the time, so quit deleting them :)
 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:354
> > +    def _convert_path(self, path):
> 
> Why is _convert_path a port thing?  Seems it's a FileSystem thing.  Maybe just a path() method in a common.filesystem.py?

_convert_path is sort of a chromium-specific thing, because it's saying for this particular usage, convert cygwin paths but nothing else. The path module doesn't have that sort of logic in it, and I'm not sure if it makes sense to do so (i.e., what are we converting to? why are we leaving win32 alone? etc.)

> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:381
> > +            # See note above in diff_image() for why we need
> > +            # _convert_path().
> 
> Did we just miss the 80c boundary? ;)
>

No, automatic text wrapping. Fixed.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:382
> > +            driver_args.append("--pixel-tests=" +
> 
> I wonder if we should have a fancy Args class which knows how to handle things like this.  A special append_path, method for example.  Donno.  Just a thought for the future.
>

Maybe. I don't feel strongly about it one way or another.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:182
> > +                else:
> > +                    return ''
> 
> WebKit has a policy of avoiding else after return (at least for our C++ code, and I think it should apply to python as well).
>

Yeah, I normally try to do this, but old habits die very hard.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:46
> > +            os.unlink(self.guard_lock_file)
> 
> Seems this should have been _guard_lock_file (but obviously that's a separate change.

Agreed.

[Dirk Pranke]

Created attachment 70945 [details]
Patch

[Dirk Pranke]

Created attachment 70946 [details]
Patch

[Dirk Pranke]

Created attachment 71094 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 71094 [details]
Patch

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

I'd like to see a new version using with_statement.  Otherwise I think this is fine.

The _Cygpath class is a bit big (does management of a singleton, as well as the nuts and bolts of talking to the process), but it's about to get a lot cleaner with "with", so it may not matter.

> WebKitTools/Scripts/webkitpy/common/system/path.py:53
> +    lock = None

Shouldn't this be _lock?  I guess the class is already _, but hte lock seems internal.

> WebKitTools/Scripts/webkitpy/common/system/path.py:58
> +        if _CygPath.lock:

I would have early returned isntead.

> WebKitTools/Scripts/webkitpy/common/system/path.py:59
> +            _CygPath.lock.acquire()

using "with" here would make the locking cleaner.  No need for the try/finally then.

> WebKitTools/Scripts/webkitpy/common/system/path.py:72
> +        _CygPath.lock.acquire()
> +        if not _CygPath.singleton:

"with" again would make this easier.

> WebKitTools/Scripts/webkitpy/common/system/path.py:81
> +        result = None
> +        try:
> +            result = _CygPath.singleton.convert(path)
> +        finally:
> +            _CygPath.lock.release()

with allows you to just return directly too.

> WebKitTools/Scripts/webkitpy/common/system/path.py:86
> +        self._child_process = None

Naming this _child might be sufficiently clear, and would save some typing.  Your call.

> WebKitTools/Scripts/webkitpy/common/system/path.py:94
> +                                               close_fds=False)

close_fds defaults to false.

> WebKitTools/Scripts/webkitpy/common/system/path_unittest.py:38
> +        if platform == 'cygwin' and sys.platform != 'cygwin':
> +            return

Sad we can't require python 2.7: http://docs.python.org/library/unittest.html#skipping-tests-and-expected-failures

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:44
>      def clean_all_lockfile(self):

sigh.  This is not english, obviously not your fault.

[Dirk Pranke]

(In reply to comment #25)
> (From update of attachment 71094 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71094&action=review
> 
> I'd like to see a new version using with_statement.  Otherwise I think this is fine.
> 

Will do. I didn't realize you could use a lock in a with_statement.

> The _Cygpath class is a bit big (does management of a singleton, as well as the nuts and bolts of talking to the process), but it's about to get a lot cleaner with "with", so it may not matter.
>

I originally had all of the singleton logic in global variables and free functions, but you said you wanted them to be class or static variables instead, so I changed this. I figured it would be best to keep as much of the logic together in a single area. What would you suggest? Two different classes? A mix of objects and global variables and free functions?
 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:53
> > +    lock = None
> 
> Shouldn't this be _lock?  I guess the class is already _, but hte lock seems internal.
>

This was left over from when convert_using_singleton() was a free function. Yes, I guess I can make this internal now.
 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:58
> > +        if _CygPath.lock:
> 
> I would have early returned isntead.
> 

Okay.

> > WebKitTools/Scripts/webkitpy/common/system/path.py:59
> > +            _CygPath.lock.acquire()
> 
> using "with" here would make the locking cleaner.  No need for the try/finally then.
>

Will do.
 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:72
> > +        _CygPath.lock.acquire()
> > +        if not _CygPath.singleton:
> 
> "with" again would make this easier.
>

Will do.
 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:81
> > +        result = None
> > +        try:
> > +            result = _CygPath.singleton.convert(path)
> > +        finally:
> > +            _CygPath.lock.release()
> 
> with allows you to just return directly too.
>

Will do.
 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:86
> > +        self._child_process = None
> 
> Naming this _child might be sufficiently clear, and would save some typing.  Your call.
>

And here I thought you always liked longer variable names ...
 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:94
> > +                                               close_fds=False)
> 
> close_fds defaults to false.
>

Hm. Thought it defaulted to true, but you're right.

[Eric Seidel (no email)]

(In reply to comment #26)
> > The _Cygpath class is a bit big (does management of a singleton, as well as the nuts and bolts of talking to the process), but it's about to get a lot cleaner with "with", so it may not matter.
> >
> 
> I originally had all of the singleton logic in global variables and free functions, but you said you wanted them to be class or static variables instead, so I changed this. I figured it would be best to keep as much of the logic together in a single area. What would you suggest? Two different classes? A mix of objects and global variables and free functions?

I'm not sure it matters.  The with thing was the biggie.  Fix that and I think the rest doesn't matter.

> > > WebKitTools/Scripts/webkitpy/common/system/path.py:86
> > > +        self._child_process = None
> > 
> > Naming this _child might be sufficiently clear, and would save some typing.  Your call.
> >
> 
> And here I thought you always liked longer variable names ...

:( I'm certainly not trying to send mixed-signals.  Maybe typing self._child_process all the time is an artifact of class mis-design.  I'm not sure.

I think we've probably beat this horse dead enough.  I can tell what the patch i doing, it's an improvement, I think we're making the code better and we should let you move forward onto other things which might depend on this. :)

[Dirk Pranke]

(In reply to comment #27)
> (In reply to comment #26)
> > > The _Cygpath class is a bit big (does management of a singleton, as well as the nuts and bolts of talking to the process), but it's about to get a lot cleaner with "with", so it may not matter.
> > >
> > 
> > I originally had all of the singleton logic in global variables and free functions, but you said you wanted them to be class or static variables instead, so I changed this. I figured it would be best to keep as much of the logic together in a single area. What would you suggest? Two different classes? A mix of objects and global variables and free functions?
> 
> I'm not sure it matters.  The with thing was the biggie.  Fix that and I think the rest doesn't matter.
> 

I agree it doesn't really matter. Still, the better I understand your design inclinations the fewer review iterations I might need in the future.

> > > > WebKitTools/Scripts/webkitpy/common/system/path.py:86
> > > > +        self._child_process = None
> > > 
> > > Naming this _child might be sufficiently clear, and would save some typing.  Your call.
> > >
> > 
> > And here I thought you always liked longer variable names ...
> 
> :( I'm certainly not trying to send mixed-signals.  Maybe typing self._child_process all the time is an artifact of class mis-design.  I'm not sure.
> 

This was half meant as a joke :) However, I do actually think that _child_process is clearer than _child, since _child could be a lot of things. In other words, I think the type (process) is more important that the name (child). If I was to use a shorter name, I would've called it _proc, not _child.

> I think we've probably beat this horse dead enough.  I can tell what the patch i doing, it's an improvement, I think we're making the code better and we should let you move forward onto other things which might depend on this. :)

:) Patch coming up shortly.

[Dirk Pranke]

Created attachment 71104 [details]
Patch

[Eric Seidel (no email)]

(In reply to comment #28)
> I agree it doesn't really matter. Still, the better I understand your design inclinations the fewer review iterations I might need in the future.

Yes, I think that the class in question could probably be split into two classes.  One to manage the lock and the process and another to do the actual talking to the process (and hiding the details of dealing with subprocess).

I think the lock stuff could almost warrent its own 3rd class, but with the "with" changes I bet it will be tiny enough to not matter.

The _Cygpath class would handle the lock, and have a pointer to the _CygpathProcess which would know how to talk to the actual process.

Or at least that's a design I might have explored were I writign this patch.  Without actually sitting down to write it, it's hard to know if such a design would be over-kill or not.

Hopefully that provided some insight.  Again, I think we'll just move-forward with this as-is and if we come back to the CYGWIN stuff and have eto add a bunch more complexity, we'll split this class then.

> This was half meant as a joke :) However, I do actually think that _child_process is clearer than _child, since _child could be a lot of things. In other words, I think the type (process) is more important that the name (child). If I was to use a shorter name, I would've called it _proc, not _child.

OK.  _child_process is certainly fine.

[Eric Seidel (no email)]

Comment on attachment 71104 [details]
Patch

LGTM.

[Dirk Pranke]

Comment on attachment 71104 [details]
Patch

Clearing flags on attachment: 71104

Committed r70012: <http://trac.webkit.org/changeset/70012>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.