#development
Title
h
hundreds-father-404
02/10/2021, 7:38 PMDo you know why we use
RefCell<Option<String>>thread_local!THREAD_PARENT_IDOption<String>task_local!RefCellMutex<Option<String>>cc @witty-crayon-22786
Also, might make sense to pair with someone on this. I'm pretty in the weeds for why the metrics aren't showing up - have made lots of progress narrowing it down! But a little unsure where to go now
w
witty-crayon-22786
02/10/2021, 7:44 PMyes… the TaskLocal isn’t “mutable”… rather than “setting” it for the task, it’s scoped for a particular future
the threadlocal on the other hand is set for a task
h
hundreds-father-404
02/10/2021, 7:45 PMGot it. That makes sense. I'm rereading https://doc.rust-lang.org/std/cell/index.html
w
witty-crayon-22786
02/10/2021, 7:45 PM@hundreds-father-404: i’m surprised that the the workunit stuff could be related here… the place where the metric is recorded asserts that a store is present, right?
there are a few parts to this problem: 1) making sure that metrics are written into the Store, and 2) making sure that something reads them out of the store, 3) making sure that 1 and 2 are happening in order
we already know that 3 isn’t the case, because of https://github.com/pantsbuild/pants/issues/11433
i can pair, but before we do, let’s discuss here for a bit
h
hundreds-father-404
02/10/2021, 7:49 PMYep, that's what I'm confused about. I've confirmed:
* This is not an issue with the cache write not happening. Via log statements, I found that 29 writes started, and 28 "finished". Yet no counters recorded. I'm not worried about that 1 missing one's race condition here.
* The code in
WorkunitStore::increment_counter()StoreWording nit, it'll help me if we can consistently call it a
WorkunitStoreStorew
witty-crayon-22786
02/10/2021, 7:51 PMthere is only one workunitstore per run
h
hundreds-father-404
02/10/2021, 7:52 PMthere is only one workunitstore per runGreat, I thought that was the case but was confused. Something I'm confused by, why aren't we using something like <Arc<Mutex>> for
WorkunitStore Copy code
#[derive(Clone)]
pub struct Context {
workunit_store: WorkunitStore,
build_id: String,
}w
witty-crayon-22786
02/10/2021, 7:52 PMok, so you’ve confirmed 1… if you add some logging near where 2 happens, is it happening in time to observe 1 ?
because the
WorkunitStoreArc<Mutex<InnerWorkunitStore>>👍 1
h
hundreds-father-404
02/10/2021, 7:54 PMwait with your #1, did you mean by
StoreWorkunitStoreStorew
witty-crayon-22786
02/10/2021, 7:54 PMWorkunitStore👍 1
h
hundreds-father-404
02/10/2021, 7:56 PMCool. Sounds good on figuring out #2 now, whether we are actually reading from the
WorkunitStorew
witty-crayon-22786
02/10/2021, 7:57 PMyep
and as i said, we already know that 3 is not happening. that’s a known bug.
(c.f. that ticket and the linked “tail tasks” thing)
but i would view what you’re currently doing is confirming that 1 and 2 are happening out of order
if “finished=True”, it’s going to be the last poll before the run completes… anything that happens after that final poll will not be captured by the StreamingWorkunitHandler
the Session and WorkunitStore continue to exist after that call… so stuff can continue to happen after the client has gone away. that’s where tail tasks / background completion of the StreamingWorkunitHandler would come in
👍 1
…or. hm, sec. i guess there is a separate method for metrics. this one: https://github.com/pantsbuild/pants/blob/477cfec6b1bf12e2983ac5d30a651cedaccf9071/src/python/pants/engine/streaming_workunit_handler.py#L66-L72
👍 1
grabbing lunch, bit if you want to pair afterward, lemme know
❤️ 1
🥪 1
h
hundreds-father-404
02/10/2021, 8:20 PMI think that first link is more relevant. Second one is only histograms it seems
But okay, trying to figure this out. In particular, I'm wondering if it'd help to somehow add a blocking call before the read to ensure the write has finished
w
witty-crayon-22786
02/10/2021, 8:20 PMthat is Tom’s tail tasks PR
👍 1
it’s linked from https://github.com/pantsbuild/pants/issues/11433
h
hundreds-father-404
02/10/2021, 8:44 PMHm, neither Tom's PR nor adding
time.sleep(4)self.run_tracker.end_run(engine_result)LocalPantsRunnerw
witty-crayon-22786
02/10/2021, 8:57 PMPossibly. But again: only one WorkunitStore, afaik. Some aggregation happens at read time, so that’s also a possibility.
I'd lean in to adding log statements to confirm that things are happening in the right order, and only then dive into whether data is being read/written different places
the counters are read here by `poll_workunits`: https://github.com/pantsbuild/pants/blob/477cfec6b1bf12e2983ac5d30a651cedaccf9071/src/rust/engine/src/externs/interface.rs#L1059-L1075
👍 1
👀 1
but it’s a “per-workunit” thing, which requires proper parent links, i think. not exactly sure how that works, but Tom knows more.
…given that there must be proper parent links for the workunits, if a task isn’t being created the right way, it wouldn’t have a parent workunit. i.e.,
Copy code
779:pub struct WorkunitStoreHandle {
780- pub store: WorkunitStore,
781- pub parent_id: Option<SpanId>,h
hundreds-father-404
02/10/2021, 9:06 PMwhereas I've confirmed the parent_id is some
w
witty-crayon-22786
02/10/2021, 9:06 PMif it were none anywhere in the chain above where the metric was recorded
(except at the root)
but even then, i don’t think that matters actually… the workunit would still end up with the metric, so i think it would still get recorded.
so yea, sorry. can ignore the parent_id angle i think.
👍 1
h
hundreds-father-404
02/10/2021, 9:08 PMMore confirmation: the counter still doesn't work if I do it immediately at the start of the
self.executor.spawn(async move))@fast-nail-55400 Stu and I figured it out! And we got it working, but there's a more general problem of dangling pointers with workunits that we can fix by leveraging Rust's borrow checker.
Thoughts on https://github.com/pantsbuild/pants/issues/11548?
f
fast-nail-55400
02/10/2021, 11:52 PM👀
added some questions to the issue