feat(prover): improve precision for prover timings in database #2418
Conversation
antoniolocascio
force-pushed
the
alocascio@wvg_time_taken
branch
3 times, most recently
from
66c24c3 to
8d2e87a
Compare
prover/prover_fri/src/utils.rs
Outdated
| @@ -101,7 +102,7 @@ pub async fn save_proof( | |||
| let mut transaction = connection.start_transaction().await.unwrap(); | |||
| transaction | |||
| .fri_prover_jobs_dal() | |||
| .save_proof(job_id, started_at.elapsed(), &blob_url) | |||
| .save_proof(job_id, time_taken, &blob_url) | |||
There was a problem hiding this comment.
This looks like a bug. If the above code takes a lot of time, we mark time_taken differently, no?
There was a problem hiding this comment.
Hmm my thinking was that I wanted to keep the logs consistent with the DB entry for the time taken. I'm not sure I understand the question. If the above code takes time it doesn't matter, time_taken is computed once.
There was a problem hiding this comment.
There's no requirement to keep the time consistent between logs. Logs are indicative of when actions happened. If you ask me, I'd add another log between the first one and the blob updated and use .elapsed() on save_proof. Sorry for being annoying on this one, but I don't want to change the semantics of our values, otherwise any other analysis I'll do in the future will be broken, given we measure time differently.
core/lib/db_connection/src/utils.rs
Outdated
| } else { | ||
| NaiveTime::from_hms_opt(hours, minutes, seconds).unwrap() | ||
| } | ||
| } | ||
| pub fn duration_to_naive_time(duration: Duration) -> NaiveTime { |
There was a problem hiding this comment.
I think you can leave this function as it was, and compute ms in the _ms function and simply add it to NaiveTime generated by the function. Something across these lines:
duration_to_naive_time(...) + NaiveTime::from_hms_milli_opt(0, 0, 0, ms).
Hope that makes sense. Additionally (since you're touching this code base), let's turn the unwraps into .expect() with some nice message.
There was a problem hiding this comment.
Right, fixed in c4973a3
Also changed the unwrap from the original function to an expect.
antoniolocascio
force-pushed
the
alocascio@wvg_time_taken
branch
from
c4973a3 to
ce5c718
Compare
|
I think we need to run |
What ❔
This PR introduces two changes to the prover database:
time_takenfor provers (inprover_jobs_fri) now is stored with millisecond precision. Note that the type of the column doesn't need to change, so no migration is needed.time_taken_wvgis added toprover_jovs_frito store the time taken by the WVG.Why ❔
These two changes are useful to measure the fine-grain performance of the prover, without needing to parse logs.
Checklist
zk fmtandzk lint.