{"action":"closed","issue":{"url":"https://api.github.com/repos/pantsbuild/pants/issues/4394","repository_url":"https://api.github.com/repos/pantsbuild/pants","labels_url":"https://api.github.com/repos/pantsbuild/pants/issues/4394/labels{/name}","comments_url":"https://api.github.com/repos/pantsbuild/pants/issues/4394/comments","events_url":"https://api.github.com/repos/pantsbuild/pants/issues/4394/events","html_url":"https://github.com/pantsbuild/pants/issues/4394","id":217396640,"node_id":"MDU6SXNzdWUyMTczOTY2NDA=","number":4394,"title":"Inlining of PathGlob expansion causes excess invalidation with the daemon","user":{"login":"stuhood","id":46740,"node_id":"MDQ6VXNlcjQ2NzQw","avatar_url":"https://avatars1.githubusercontent.com/u/46740?v=4","gravatar_id":"","url":"https://api.github.com/users/stuhood","html_url":"https://github.com/stuhood","followers_url":"https://api.github.com/users/stuhood/followers","following_url":"https://api.github.com/users/stuhood/following{/other_user}","gists_url":"https://api.github.com/users/stuhood/gists{/gist_id}","starred_url":"https://api.github.com/users/stuhood/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stuhood/subscriptions","organizations_url":"https://api.github.com/users/stuhood/orgs","repos_url":"https://api.github.com/users/stuhood/repos","events_url":"https://api.github.com/users/stuhood/events{/privacy}","received_events_url":"https://api.github.com/users/stuhood/received_events","type":"User","site_admin":false},"labels":[{"id":268094252,"node_id":"MDU6TGFiZWwyNjgwOTQyNTI=","url":"https://api.github.com/repos/pantsbuild/pants/labels/engine","name":"engine","color":"fbca04","default":false},{"id":382187620,"node_id":"MDU6TGFiZWwzODIxODc2MjA=","url":"https://api.github.com/repos/pantsbuild/pants/labels/pantsd","name":"pantsd","color":"d4c5f9","default":false},{"id":422137276,"node_id":"MDU6TGFiZWw0MjIxMzcyNzY=","url":"https://api.github.com/repos/pantsbuild/pants/labels/performance","name":"performance","color":"0052cc","default":false}],"state":"closed","locked":false,"assignee":{"login":"illicitonion","id":1131704,"node_id":"MDQ6VXNlcjExMzE3MDQ=","avatar_url":"https://avatars0.githubusercontent.com/u/1131704?v=4","gravatar_id":"","url":"https://api.github.com/users/illicitonion","html_url":"https://github.com/illicitonion","followers_url":"https://api.github.com/users/illicitonion/followers","following_url":"https://api.github.com/users/illicitonion/following{/other_user}","gists_url":"https://api.github.com/users/illicitonion/gists{/gist_id}","starred_url":"https://api.github.com/users/illicitonion/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/illicitonion/subscriptions","organizations_url":"https://api.github.com/users/illicitonion/orgs","repos_url":"https://api.github.com/users/illicitonion/repos","events_url":"https://api.github.com/users/illicitonion/events{/privacy}","received_events_url":"https://api.github.com/users/illicitonion/received_events","type":"User","site_admin":false},"assignees":[{"login":"illicitonion","id":1131704,"node_id":"MDQ6VXNlcjExMzE3MDQ=","avatar_url":"https://avatars0.githubusercontent.com/u/1131704?v=4","gravatar_id":"","url":"https://api.github.com/users/illicitonion","html_url":"https://github.com/illicitonion","followers_url":"https://api.github.com/users/illicitonion/followers","following_url":"https://api.github.com/users/illicitonion/following{/other_user}","gists_url":"https://api.github.com/users/illicitonion/gists{/gist_id}","starred_url":"https://api.github.com/users/illicitonion/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/illicitonion/subscriptions","organizations_url":"https://api.github.com/users/illicitonion/orgs","repos_url":"https://api.github.com/users/illicitonion/repos","events_url":"https://api.github.com/users/illicitonion/events{/privacy}","received_events_url":"https://api.github.com/users/illicitonion/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/pantsbuild/pants/milestones/11","html_url":"https://github.com/pantsbuild/pants/milestone/11","labels_url":"https://api.github.com/repos/pantsbuild/pants/milestones/11/labels","id":2811452,"node_id":"MDk6TWlsZXN0b25lMjgxMTQ1Mg==","number":11,"title":"Daemon Beta","description":"","creator":{"login":"kwlzn","id":1883523,"node_id":"MDQ6VXNlcjE4ODM1MjM=","avatar_url":"https://avatars0.githubusercontent.com/u/1883523?v=4","gravatar_id":"","url":"https://api.github.com/users/kwlzn","html_url":"https://github.com/kwlzn","followers_url":"https://api.github.com/users/kwlzn/followers","following_url":"https://api.github.com/users/kwlzn/following{/other_user}","gists_url":"https://api.github.com/users/kwlzn/gists{/gist_id}","starred_url":"https://api.github.com/users/kwlzn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kwlzn/subscriptions","organizations_url":"https://api.github.com/users/kwlzn/orgs","repos_url":"https://api.github.com/users/kwlzn/repos","events_url":"https://api.github.com/users/kwlzn/events{/privacy}","received_events_url":"https://api.github.com/users/kwlzn/received_events","type":"User","site_admin":false},"open_issues":1,"closed_issues":26,"state":"closed","created_at":"2017-10-03T18:18:56Z","updated_at":"2018-07-06T22:33:44Z","due_on":null,"closed_at":"2018-06-18T23:37:30Z"},"comments":6,"created_at":"2017-03-27T22:42:37Z","updated_at":"2018-07-06T22:33:44Z","closed_at":"2018-07-06T22:33:44Z","author_association":"MEMBER","body":"Inlining of `PathGlobsExpansion` causes `Snapshots` to be more fragile than necessary. In particular, because `VFS::expand` recursively walks from the root down to the relevant directories without memoizing `PathGlobsExpansion`s, any change in the root invalidates _all_ `Snapshots` from a changed path up to the root of the repo (but not the directory listings and stats).\r\n\r\nThis significantly affects warm performance in the presence of the daemon.\r\n\r\nWe should experiment with memoizing (portions of?) `PathGlobsExpansion` in the `Graph` (similar to what we do for `ReadLink`/`Scandir`), and see how much it affects cold/warm performance.\r\n\r\nNB: As a wildcard: it's possible that fixing #4558 would be a better approach than increasing memoization would be.\r\n\r\n----\r\n\r\nThe relevant codepath centers around https://github.com/pantsbuild/pants/blob/cdba694c38b4d60342c75a44ff97c5642429e585/src/rust/engine/fs/src/lib.rs#L570-L576 . When we call that method from the engine (here: https://github.com/pantsbuild/pants/blob/01bcda6e3c352d99dc6a583ed57295d3a4c5afe8/src/rust/engine/src/nodes.rs#L913-L940), the `VFS` in question is https://github.com/pantsbuild/pants/blob/01bcda6e3c352d99dc6a583ed57295d3a4c5afe8/src/rust/engine/src/nodes.rs#L73 , which has the effect of memoizing/recording `ReadLink` and `Scandir` calls in the `Graph` (which later allows for filesystem invalidation).\r\n\r\nThe effect of memoizing `ReadLink` and `Scandir` (but none of the rest of the other steps of a `PathGlobsExpansion`) is that a `Snapshot` `Node` in the `Graph` is flat: the `Snapshot` depends directly on the syscalls, and to recompute it the entire `VFS::expand` call reruns.\r\n\r\n----\r\n\r\nWhen we fix this, it would be good to get a simple benchmark in place on the rust side. But it's also important to directly measure the performance of:\r\n* cold `./pants list ::`\r\n* warm `./pants list ::` after touching one file at the root of the repo"}}
{"actor":{"display_login":"stuhood"}}