Bug 50635

Created attachment 75825 [details] proposed patch

Attachment 75825 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75825 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75825 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75825 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 75825 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=75825&action=review Well, instead this causes us to stat the file system for each path. I guess we're doing so right above, so the inode should be cached... Would this be faster/just as fast if we just compiled a regexp for the path first? How did you perf test this? How much speedup did you see? > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:723 > + if not os.path.isfile(path): I guess I would have reversed this if to be positive instead of negative. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:725 > + result = [ test for test in self._full_test_list if test.startswith(path) ] I don't think PEP8 says we put extra spaces around [ ] like this.

(In reply to comment #6) > > How did you perf test this? How much speedup did you see? > This is easy to perf :) Just try new-run-webkit-tests --platform=gtk with and without the patch. We (Alexg and I) profiled this with the python hotshot profiler. Without the patch parsing the expectations takes about 20 seconds here and with the patch less than 2 seconds. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:723 > > + if not os.path.isfile(path): > > I guess I would have reversed this if to be positive instead of negative. > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:725 > > + result = [ test for test in self._full_test_list if test.startswith(path) ] > > I don't think PEP8 says we put extra spaces around [ ] like this. OK I can fix those. Thanks for the review

Created attachment 76320 [details] updated patch

Attachment 76320 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKitTools/ChangeLog', u'WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py']" exit_code: 1 WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:739: missing whitespace after ',' [pep8/E231] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 76377 [details] updated patch and "style" fixed!

Comment on attachment 76377 [details] updated patch I assume you ran the unittests for this. If not, please make sure to do so before committing. Also, it would be nice if you could add a test for this case. The closest thing I could find was test_precedence, but that doesn't test the case of a file being listed without it's parent directory also being listed. Minor edge case, but more tests make me feel safer. :)

(In reply to comment #11) > (From update of attachment 76377 [details]) > I assume you ran the unittests for this. If not, please make sure to do so before committing. > Yes I did check that before sending the patch for review. > Also, it would be nice if you could add a test for this case. The closest thing I could find was test_precedence, but that doesn't test the case of a file being listed without it's parent directory also being listed. Minor edge case, but more tests make me feel safer. :) Hum now I figured out my patch should rely on port's filesystem API :) Will send a new patch with a test!

Created attachment 76397 [details] updated patch

Nice catch! I knew there had to be a reason chromium's parsing was getting quadratically slower but I hadn't had the time to look into it. The change basically looks fine but I would make one minor modification. We already know whether the path points to a directory or a file from the check on line 732, so you can merge the two if blocks together and save yourself an extra os call. Seems like we could probably still speed up the directory/category branch of the loop by switching to a more tree-based storage mechanism for the file list, or by revamping the calling code to do a merge join instead of a nested-loop join but it's probably not worth it for now, so I would leave the code as is since it's a lot more obviously correct. You could consider adding a #FIXME that there's an opportunity for a speed up here. As far as using a regexp goes, I'd be more than a little surprised if a regexp was faster than a string prefix match. -- Dirk

Comment on attachment 76397 [details] updated patch r- per Dirk's comments. Specifically, making use of the is_dir check above, which will mean we don't need to add isfile to port either.

Comment on attachment 76377 [details] updated patch Cleared Ojan Vafai's review+ from obsolete attachment 76377 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 76532 [details] updated patch

Attachment 76532 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKitTools/ChangeLog', u'WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py', u'WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations_unittest.py', u'WebKitTools/Scripts/webkitpy/layout_tests/port/test.py']" exit_code: 1 WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:222: missing whitespace around operator [pep8/E225] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 76533 [details] updated patch

Comment on attachment 76533 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=76533&action=review > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:742 > + result = [test for test in self._full_test_list if test.startswith(path)] nit: I would make this an early return, then you can drop the "else:" line. > WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:214 > + if path.find('.') != -1: This is fine for now given that this is just for testing, but in the real webkit tree, we have the CSS2.1 directory, which we may want to add tests for in the future. Please add a FIXME to make this work for that case.

Committed r74036: <http://trac.webkit.org/changeset/74036>

Comment on attachment 76533 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=76533&action=review >> WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:214 >> + if path.find('.') != -1: > > This is fine for now given that this is just for testing, but in the real webkit tree, we have the CSS2.1 directory, which we may want to add tests for in the future. Please add a FIXME to make this work for that case. In particular, since the list of files and directories is fixed and small in this case, all you are doing is speeding up the check in line 224, but I doubt that produces a noticeable speedup.

(In reply to comment #22) > (From update of attachment 76533 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76533&action=review > > >> WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:214 > >> + if path.find('.') != -1: > > > > This is fine for now given that this is just for testing, but in the real webkit tree, we have the CSS2.1 directory, which we may want to add tests for in the future. Please add a FIXME to make this work for that case. > > In particular, since the list of files and directories is fixed and small in this case, all you are doing is speeding up the check in line 224, but I doubt that produces a noticeable speedup. I had to make this change to actually make the tests pass... The noticeable speedup is in the real tests expectations parsing, eg. avoiding the expensive iteration over the *BIG* list of tests, when possible. :)

FWIW, this cut parse expectations time on the chromium linux bot from 9 seconds to 2.

(In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 76533 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=76533&action=review > > > > >> WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:214 > > >> + if path.find('.') != -1: > > > > > > This is fine for now given that this is just for testing, but in the real webkit tree, we have the CSS2.1 directory, which we may want to add tests for in the future. Please add a FIXME to make this work for that case. > > > > In particular, since the list of files and directories is fixed and small in this case, all you are doing is speeding up the check in line 224, but I doubt that produces a noticeable speedup. > > I had to make this change to actually make the tests pass... > > The noticeable speedup is in the real tests expectations parsing, eg. avoiding the expensive iteration over the *BIG* list of tests, when possible. :) Oh, yeah, obviously there's a big speedup in the real parsing. I'm just surprised that you would need to make a change to test.py for the tests to pass. I'll have to look into that.

(In reply to comment #25) > > > >> WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:214 > > > >> + if path.find('.') != -1: > > > > > > > > This is fine for now given that this is just for testing, but in the real webkit tree, we have the CSS2.1 directory, which we may want to add tests for in the future. Please add a FIXME to make this work for that case. > > > > > > > I had to make this change to actually make the tests pass... > > > > > I'm just surprised that you would need to make a change to test.py for the tests to pass. I'll have to look into that. I guess I was justifiably surprised. That line actually causes test-webkitpy to fail for me, because my source tree is rooted in a directory with a "." in it (e.g. /src/foo.bar/WebKit/WebKitTools/...). However, if I comment out those two lines, it passes for me. What fails for you without those lines?

(In reply to comment #26) > (In reply to comment #25) > > > > >> WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:214 > > > > >> + if path.find('.') != -1: > > > > > > > > > > This is fine for now given that this is just for testing, but in the real webkit tree, we have the CSS2.1 directory, which we may want to add tests for in the future. Please add a FIXME to make this work for that case. > > > > > > > > > > I had to make this change to actually make the tests pass... > > > > > > > > I'm just surprised that you would need to make a change to test.py for the tests to pass. I'll have to look into that. > > I guess I was justifiably surprised. That line actually causes test-webkitpy to fail for me, because my source tree is rooted in a directory with a "." in it (e.g. /src/foo.bar/WebKit/WebKitTools/...). However, if I comment out those two lines, it passes for me. What fails for you without those lines? Oh I see what happened :) I made that change before doing the early return in the expand_tests code. It was needed in that case because path_isdir() was returning False and the wrong code path was taken, making two test-cases fail. With the early return in expand_tests that dummy code in test.py is no longer needed.

(In reply to comment #27) > > Oh I see what happened :) I made that change before doing the early return in the expand_tests code. It was needed in that case because path_isdir() was returning False and the wrong code path was taken, making two test-cases fail. > > With the early return in expand_tests that dummy code in test.py is no longer needed. That makes sense. I remember doing something similarly before. Of course, my testing has reviewed that test.py is sensitive to where it is being run from (using a real layout_tests_dir()), which it shouldn't be. So, I'll upload a separate patch that fixes that and removes this line.

(In reply to comment #28) > (In reply to comment #27) > > > > Oh I see what happened :) I made that change before doing the early return in the expand_tests code. It was needed in that case because path_isdir() was returning False and the wrong code path was taken, making two test-cases fail. > > > > With the early return in expand_tests that dummy code in test.py is no longer needed. > > That makes sense. I remember doing something similarly before. Of course, my testing has reviewed that test.py is sensitive to where it is being run from (using a real layout_tests_dir()), which it shouldn't be. > > So, I'll upload a separate patch that fixes that and removes this line. Oh, and add some tests to make sure that we handle directories with "." in them properly.