Optimise the manifest update #12553

jgraham · 2018-08-17T15:50:46Z

Not ready to merge etc. Mostly concerned with making no-op updates as fast as possible.

gsnedders

I tend to be a bit weary of fixing performance just by writing a bunch of caches versus fixing the underlying performance issues, which is essentially what this does for gitignore.

On the mtime side, I wonder if we should be using the user's cache directory ($XDG_CACHE_HOME or whatever), rather than introducing our own cache files in random places in the filesystem?

gsnedders · 2018-08-20T16:13:47Z

tools/manifest/commands.json

@@ -1,6 +1,5 @@
 {"manifest":
 {"path": "update.py", "script": "run", "parser": "create_parser", "help": "Update the MANIFEST.json file",
-  "virtualenv": false},
+  "virtualenv": false, "install": ["ujson"]},


You'd previously been against adding non-vendored dependencies for manifest/lint? Is the intention to require it for wpt manifest but not using manifest as a package? How are we going to test it works under both environments?

I didn't mean to add this. I intend to use ujson if it's available but not to require it. For a no-op update the time saving is almost exactly cancelled by the time to setup the virtualenv.

gsnedders · 2018-08-20T16:15:25Z

.gitignore

@@ -28,5 +22,5 @@ config.json
 scratch
 .idea/
 .vscode/
-.DS_Store
-*.rej
+.DS_Store/


.DS_Store is a file and not a directory?

gsnedders · 2018-08-20T16:15:37Z

.gitignore

-.DS_Store
-*.rej
+.DS_Store/
+*.rej


You've removed end terminating new line here?

gsnedders · 2018-08-21T13:30:26Z

tools/gitignore/gitignore.py


 end_space = re.compile(r"([^\\]\s)*$")


-def fnmatch_translate(pat, allow_component_only=True):
+def fnmatch_translate(pat):


Given you've already added an external dependency, is there any reason not to just move gitignore to use a better regex impl that does O(n) matching?

gsnedders · 2018-08-21T13:31:13Z

tools/manifest/vcs.py

+            self.data[key] = value
+
+
+def walk(root):


Is there any reason we're doing this rather than using the scandir package and returning DirEntrys?

Just because it's one less required dependency, and a C one at that. Also since we typically require the stat data anyway it's not clear how much of a win it will be.

jgraham · 2018-08-24T11:10:26Z

I think this is bascially ready for review. The branch is a bit of a mess and the cache stuff probably needs better tests, but for my sanity I think I need to work on something else for a bit :)

Ms2ger · 2018-09-06T11:32:50Z

tools/gitignore/gitignore.py

    else:
-        any_char = "."
+        # By default match the entire path up to a /
+        # but if / doesn't appear in the pattern we will mark is as


We will what now?

Ms2ger · 2018-09-06T11:35:14Z

tools/wpt/wpt.py

-                pdb.post_mortem()
-            else:
-                raise
+            raise


Do you still need the try/except?

…t update

We end up with a lot of rules like (?:.*)/.*\.ext which are basically trying to find the last component in a path and match against that. These are rather slow to run so the easiest thing tdo is just pass in the last component of the path when we know that's the only thing the rule can match. The changes to surrounding code to use this API will be made in future commits.

This updates the gitignore implemenation to take input like os.walk but with additional stat data for the files. It also makes several useful optimistaions: * Avoid using regex when just matching a literal * Identify patterns that can only match the final component of a path and run those against that component rather than the full path. * Add the possibility of providing a dictionary of paths to gitignore statuses as a cache. This dramatically reduces the amount of time we spend in gitignore processing when updating the manifest.

When processing the manifest using the worktree, instead of reading all files to see if the content changed, instead only process files where the mtime has been updated since the previous run. Also cache the result of running gitignore, so we can save a couple of seconds processing the gitignore rules.

Compared to the normal os.walk this has a couple of differences: * It returns lists of (name, stat) for filenames and directories, allowing callers to reuse the stat data without going back to the system to re-request it. * Directories are always returned as paths relative to the root, and the root itself is returned as the empty string. * It is non-recursive. There are also a few features missing that aren't required for our use cases.

The caches weren't being invalidated when the manifest itself changed. To fix this the manifest itself has to be written before the cache and the cache has to include data about the manifest that it's associated with (the mtime and path are used for this purpose). To make all this work requires a single method that can load the manifest, update it, write the manifest and write the caches. Therefore we introduce a single load_and_update method that is intended to replace all previous use of the load() or update() methods (and as a bonus handles manifest version mismatches in a single place).

jgraham · 2018-10-10T14:30:25Z

Since there's basically no traction on getting this landed here, I created gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1497898 with these patches applied to mozilla-central. I really don't mind which way we land this, but it is important to me that we either land this or something functionally equivalent (i.e. something that can drop the zero-change manifest update time using the worktree to around 1s).

foolip · 2018-10-10T15:18:29Z

Please ignore the CircleCI failure on this PR. I was experimenting with CircleCI on a branch and did not expect it to show up on all PRs. I've revoked access for CircleCI, and it should be possible to merge even with this spurious failure. Please let me know if there are problems.

jgraham · 2018-11-19T14:59:08Z

Landed via #13618

jgraham added the do not merge yet label Aug 17, 2018

wpt-pr-bot added infra lint manifest gitignore labels Aug 17, 2018

wpt-pr-bot requested a review from gsnedders August 17, 2018 15:50

gsnedders mentioned this pull request Aug 20, 2018

Use --no-download when uploading manifest from Travis #12560

Closed

jgraham force-pushed the manifest_optimise branch from c6facec to e124c3a Compare August 20, 2018 18:35

wpt-pr-bot added the wptrunner The automated test runner, commonly called through ./wpt run label Aug 20, 2018

gsnedders reviewed Aug 21, 2018

View reviewed changes

jgraham force-pushed the manifest_optimise branch from e124c3a to 8d76637 Compare August 23, 2018 13:05

wpt-pr-bot added the wpt label Aug 23, 2018

jgraham force-pushed the manifest_optimise branch from 8d76637 to 0820607 Compare August 23, 2018 14:44

wpt-pr-bot added the ci label Aug 23, 2018

jgraham force-pushed the manifest_optimise branch from 0ca6eee to 99c1701 Compare August 24, 2018 10:22

jgraham removed the do not merge yet label Aug 30, 2018

Ms2ger reviewed Sep 6, 2018

View reviewed changes

jgraham force-pushed the manifest_optimise branch from 99c1701 to 6bb5621 Compare September 6, 2018 12:06

Cactusmachete and others added 11 commits October 10, 2018 11:44

Use testfile mtimes to pre-filter files iterated over for the manifes…

f2fd38f

…t update

WIP

f8f8c3d

Update the .gitignore file

7917de4

Update the lint to the new gitignore API

94724e4

Fix the .gitignore rules

a5b0a39

Pass the manifest file directly into wpttest.from_manifest

18a6977

Update manifest before tests

7ccdbbe

jgraham force-pushed the manifest_optimise branch from 6bb5621 to 7ccdbbe Compare October 10, 2018 14:31

jgraham mentioned this pull request Nov 13, 2018

tools/wpt testsuite exceptionally slow on Windows #12934

Closed

jgraham closed this Nov 19, 2018

wpt-pr-bot requested a review from jugglinmike November 19, 2018 14:59

jugglinmike mentioned this pull request Nov 19, 2018

Do not react to closed pull requests web-platform-tests/wpt-pr-bot#53

Closed

gsnedders deleted the manifest_optimise branch January 24, 2020 18:07

gsnedders restored the manifest_optimise branch January 24, 2020 18:47

foolip deleted the manifest_optimise branch April 14, 2021 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimise the manifest update #12553

Optimise the manifest update #12553

jgraham commented Aug 17, 2018

gsnedders left a comment

gsnedders Aug 20, 2018

jgraham Aug 21, 2018

gsnedders Aug 20, 2018

gsnedders Aug 20, 2018

gsnedders Aug 21, 2018

gsnedders Aug 21, 2018

jgraham Aug 21, 2018

jgraham commented Aug 24, 2018

Ms2ger Sep 6, 2018

Ms2ger Sep 6, 2018

jgraham commented Oct 10, 2018

foolip commented Oct 10, 2018

jgraham commented Nov 19, 2018

Optimise the manifest update #12553

Optimise the manifest update #12553

Conversation

jgraham commented Aug 17, 2018

gsnedders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgraham commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgraham commented Oct 10, 2018

foolip commented Oct 10, 2018

jgraham commented Nov 19, 2018