Hi Frank, On Fri, Apr 23, 2021 at 03:43:09PM -0400, Frank Ch. Eigler via Elfutils-devel wrote: > PR27701: debuginfod client: encourage reused debuginfod_client objects > > Client objects now carry long-lived curl handles for outgoing > connections. This makes it more efficient for multiple sequential > queries, because the TCP connections and/or TLS state info are kept > around awhile, avoiding O(100ms) setup latencies. debuginfod is > adjusted to take advantage of this for federation. Other clients > should gradually do this too, perhaps including elfutils itself (in > the libdwfl->debuginfod_client hooks). > > A large gdb session with 117 debuginfo downloads was observed to run > twice as fast (45s vs. 1m30s wall-clock time), just in nuking this > extra setup latency. This was tested via a debuginfod intermediary: > it should be even faster once gdb reuses its own debuginfod_client.
This is really nice. We should indeed also reuse a debuginfod_client handle in libdwfl for a Dwfl or Dwfl_Module when we figure out/document the multi-thread use case. The way debuginfod uses it is multi-thread safe since it makes sure only one thread at a time uses the same debuginfod_client handle. The code looks good. I have only two comments: > @@ -1095,6 +1087,11 @@ debuginfod_query_server (debuginfod_client *c, > > /* general purpose exit */ > out: > + /* Reset sent headers */ > + curl_slist_free_all (c->headers); > + c->headers = NULL; > + c->user_agent_set_p = 0; > + > /* Conclude the last \r status line */ > /* Another possibility is to use the ANSI CSI n K EL "Erase in Line" > code. That way, the previously printed messages would be erased, [...] > @@ -175,8 +177,8 @@ of the client object. > > .SS "HTTP HEADER" > > -Before a lookup function is initiated, a client application may > -add HTTP request headers to future downloads. > +Before each lookup function is initiated, a client application may > +add HTTP request headers. These are reset after each lookup function. Why do we need to reset the headers? Is that because we don't have debuginfod_remove_http_header? Should we maybe add that (or a reset_http_header)? > @@ -90,10 +113,6 @@ wait_ready() > done; > > if [ $timeout -eq 0 ]; then > - echo "metric $what never changed to $value on port $port" I think we want to keep this message ^ it helps with knowing which exact metric timed out. > - curl -s http://127.0.0.1:$port/metrics > - echo "logs for debuginfod with port $port" > - cat vlog$port > exit 1; > fi > } Cheers, Mark
