https://pantsbuild.org/ logo
Join Slack
Powered by
pantsd and performance!: re <https://github.com/pa...
# development
w
@early-needle-54791 : It looks like there might be an issue with the first line for master... that should be 
list ::
, yea?
If 
list ::
for master/branch ends up with a similar ratio to the others (around 14%?), I think that this is good to go from a pantsd users (ie, Twitter) perspective... and if it doesn't, then you might go looking for more things that we're watching unexpectedly
But we should talk to Toolchain about whether they are on a path to enabling pantsd for their users, and thus willing to take this hit. If they're not, I think we'd need to put it behind a flag that disabled it unless pantsd was enabled.
h
Awesome! Is the goal to completely remove watchman in favor of this?
w
yep.
💯 2
cc @happy-kitchen-89482, @enough-analyst-54434: ^
i can summarize the question / expected path forward on https://github.com/pantsbuild/pants/issues/4999 if that would be helpful.
👍 1
h
That would be helpful, thanks!
w
@early-needle-54791: i expect that you're hackweeking, but i'll be in meetings for the next two hours or so. if you get a chance to summarize the question and next steps on #4999 before i do, please do.
and if you're hackweeking, don't worry about it
it's possible that the answer is to do what the #4999 description describes, and add flags that allow for enabling 1) watchman only, 2) watchman+notify, 3) notify only ... in which case the next question will be "in this PR, or in the followup PR?"
e
Taking a stab at summarizing the next steps / open questions.
❤️ 1
w
@early-needle-54791: hey, still not quite sure about that benchmark result. i see a ratio of 2.8 seconds for 
contrib
to 4.8 seconds for 
::
in master, so it doesn't seem right that you were seeing 
11.64s
for 
contrib
and 
11.57s
for 
::
and at an intuitive level, it doesn't really make sense
e
you mean on master?
w
yes
(for single runs, but i expect it to scale up to 5 runs)
e
my hypothesis was just that without notify everything is dominated by startup time
and the scandir time is completely negligible
w
would you mind re-running the "5 runs of 
list ::
on master" line?
e
sure
w
it isn't on my machine, heh
e
master isnt on ur machine?
w
^ that is not the case on my machine for master
listing something smaller takes less time, afaict.
e
oh
i may have had the --{no-}enable-pantsd removed 🤦‍♂️
eh still its saying 18 seconds for 5 laps of ::
( repeat 5; do; ./v2 --no-enable-pantsd list ::; done; ) 20.89s user 9.56s system 165% cpu 18.433 total
lets try src/:: again
errr 
( repeat 5; do; ./v2 --no-enable-pantsd list src/::; done; )  13.20s user 5.71s system 131% cpu 14.400 total
about 1 sec faster per run
to limit it to src/::
But you’re right the numbers up there seem inconsistent w/r/t ::
w
hm. i'll run the whole thing.
e
better CPU usage tho, so probably better parallelism not waiting for the lock on the watcher
Also, nothing is jumping out at me that should obviously be ignored, or has enough files to even matter.
w
so... the notify crate appears to be depressingly inefficient.
😢 1
(...at least on OSX...)
it starts and stops an event loop every time you watch something new. argh.
e
Are we "using it wrong" aka can you just watch the buildroot wholesale and then filter events you don't want from there?
👍 2
w
possibly, yea. it certainly seems like the expectation for OSX was not that folks would add files to watch individually. ... on Linux it looks like it should be much more performant.
so, next question is how the recursive mode is implemented.
...it's implemented by walking the filesystem and watching everything for linux. on OSX it looks fsevents is per-filesystem or per-disk... so recursive is cheap.
interesting. so... i think that for this to not suck we'd need different strategies per OS. that's unfortunate, but ... possible.
@early-needle-54791: so: sorry for the goose chase. based on the above, i think that where we are on this is that we need per-OS behavior, akin to https://github.com/notify-rs/notify/blob/eed64ac9088ec1aab5c4710ef7232f0fbee49e0a/src/lib.rs#L186-L191
on OSX, it's cheap to watch the whole thing, so we'd want to recursively watch the root, period. and on Linux we'd want to continue to use the implementation we have now.
i'm confirming the OSX aspect now.
...yea, confirmed. the recursive watch is cheap on OSX.
so, as awkward as that is, i think that it is a clear path forward. and just a matter of trying to contain the logic to 
<http://watch.rs|watch.rs>
to the extent possible.
👍 1
e
Okay that makes sense. So on OSX we would effectively ignore calls to watch() at runtime, and just watch the build_root once at startup?
seems like that wouldn’t be too much trouble.
w
Yep
e
I had been going over the inotify.rs module looking for where it was spending so much time, but of course its in the fsevent.rs module 🤦‍♂️
The only reason I can think of that they would be implemented so differently is because of limit/strengths of the underlying APIs
w
Yep. For sure. Fsevents on osx is "per filesystem or drive", so recursive is just filtering the relevant stuff out ... while Linux is per path
👍 1
e
new timings on the feature branch after recursive watch 
( repeat 5; do; ./v2 -lerror --no-enable-pantsd list ::; done; )  18.61s user 7.07s system 148% cpu 17.233 total
🔥 1
😅
thanks for the help stu
❤️ 1
Side note, can you guys think of a reason we would want to watch hidden files and directories in the build root? only think I can think of is config files ?
ignoring events from hidden files would help avoid unncessary full build root invalidations. i.e when something gets added .pants.d or w/e
I’m going to leave it as is watching hidden files, and we can focus on optimizing invalidation later.
This probably needs some more thorough discussion.
👍 1
w
re: skipping invalidation for ignored files: ok with it going in an immediate followup. but i think that it will actually need to be immediate, because "an invalidation event for anything under `.pantsd`"... is going to be very significant.
👍 1
but it should be based on 
pants_ignore
rather than heuristics about hidden-ness
👍 1
e
files under .pants.d shouldn’t invalidate the build_root, and should match any node fs_subjects tho.
I agree it needs to be fixed btw.
w
understood. but there will be hundreds of thousands of events arriving during v1 builds.
e
yeah….
w
and the current implementation of 
Graph::invalidate_from_roots
is ... not optimized for that.
e
Good point. Idk, maybe I should fix this now then
w
and shouldn't need to be, really... skipping events that can't be relevant makes sense.
e
Do you think its safe to do in a follow up then?
how many people are running v1 compiles from bleeding master, very few I would assume, except for where v1 is used in CI
w
@early-needle-54791: it might step on Greg's toes a bit (via https://github.com/pantsbuild/pants/pull/9310), but the way to do that i think would be to move calculation of our ignore patterns out of here: https://github.com/pantsbuild/pants/blob/45e3f0ea5e69fb6a59eed02c6474cb87c18001ba/src/rust/engine/fs/src/lib.rs#L481-L508 ... and up a level to context/Core, i think
h
One pattern I like to do is to have the followup immediately queued so that you can put it up right away. That way you still can avoid scope creep in the original PR
e
Yeah that makes sense, and thats what I want to avoid.
w
yea. Eric's suggestion is good. let's get that drafted as a followup maybe.
e
I’d also really like to avoid having to rebase the PR again…
w
yep.
e
which to me means merging sooner rather than later. Not sure how long the proper ignore PR will take.
w
if it will take a while, what you might deal with instead is a revert and re-land
e
thats true.
w
so, i think that this is a spectrum.
we don't have to wait until the ignore patch is perfect.
can dial that knob.
h
Do you expect merge conflicts the next few hours? Would probably be good to draft the followup before merging so that you have a solid idea of the scope. Maybe, that followup ends up being way more complex than you thought and would take 3 days so you have to revert, which isn’t ideal Nb that John has been planning a release for a few days (blocked by Pex upgrade which he’s close to landing), and I’m on release duty on Friday. This would need to be fixed before the release. If John releases today, for example, that would mean reverting your PR today. If we wait til Friday, you would have 3 days (We’re 5 days behind on the release already, so we probably shouldn’t wait too much longer)
w
but i think that there might actually be a significant risk of this being a bottleneck.
👍 1
e
So waiting for the --pants-gitignore pr seems like the right option in the long run, but in lieu of blocking on that this is going to use the pants-ignore option as a first pass toward getting it right?
And if the --pants-gitignore is going to do the right thing eventually can we eliminate ~90% of the problem by naively ignoring .pants.d ?
w
@early-needle-54791: oh, sorry: to be clear, i think that this is orthogonal to whether pants itself propagates gitignore patterns into what it ignores (beyond the fact that it is likely to conflict)
e
I’d rather not duplicate work / step on toes if I don’t have to.
w
pants_ignore is absolutely/already the thing to use
👍 1
Open in Slack
PreviousNext
Powered by