Suppress WorkerTracer warning for duplicate alarm requests#5685
Conversation
70e6cc5 to
fe6af85
Compare
fhanau
left a comment
There was a problem hiding this comment.
Code itself LGTM, but there is one complication with downstream usage: We support marking a WorkerTracer as unused, but that does not extend to the RPC equivalent, which is set to just have a no-op markUnused function, it would have to propagate the unused signal. However, that might just be fine if the duplicate alarm scenario cannot happen under the RPC case as you suggested? We'd have to support it in the future though if we end up marking tracers as unused in more cases.
CodSpeed Performance ReportMerging #5685 will improve performances by 9.56%Comparing Summary
Benchmarks breakdown
Footnotes
|
Exactly! |
1e160c1 to
d3ade51
Compare
With #5685, we managed to reduce the volume of "destructed WorkerTracer" warnings by >80% by eliminating it in the case of duplicate alarm events. Another case where this happens are R2 API calls where we create a new WorkerInterface for the R2 call before completing error checking, as seen in the r2-test itself. We can easily avoid this by moving getHttpClient() calls behind error checks. While this PR is not aiming for completion I also cleaned up a call in web-socket.c++ that may be susceptible to the same issue.
No description provided.