Conversation
mstemm
force-pushed
the
improve-state-switches
branch
3 times, most recently
from
1c5c41c to
f8d7195
Compare
userspace/libsinsp/threadinfo.h
Outdated
| vector<string> m_env; ///< Environment variables | ||
| string m_env_str; // m_env combined, separated by '\0', capped to SCAP_MAX_ENV_SIZE | ||
| vector<pair<string, string>> m_cgroups; ///< subsystem-cgroup pairs | ||
| string m_cgroups_str; // m_cgroups combined, separated by '\0', cappped to SCAP_MAX_CGROUPS_SIZE |
There was a problem hiding this comment.
cgroups have already a pretty big memory impact, doubling it worries me. I would test memory usage difference with an high number of threads (40k) for example.
A possible alternative on my mind can be have just strings as storage and just at getter time, parse them and return the expected value to the caller.
mstemm
force-pushed
the
improve-state-switches
branch
2 times, most recently
from
cf61245 to
6c76eec
Compare
|
Ok, this version is ready for review. I avoided storing a copy of the args/env/cgroups by using iovecs when writing the sinsp_threadinfo to the capture file. |
mstemm
force-pushed
the
improve-state-switches
branch
2 times, most recently
from
d7dbb41 to
0028be9
Compare
userspace/libscap/scap.h
Outdated
| size_t iov_len; /* Number of bytes to transfer */ | ||
| }; | ||
| #else | ||
| #include <sys/uio.h> |
There was a problem hiding this comment.
The function declaration only uses pointers, so let's avoid includes in scap.h. Forward declare this and include/define iovec in scap_savefile.c
userspace/libsinsp/threadinfo.cpp
Outdated
| std::string &rem) | ||
| { | ||
| (*iovcnt)++; | ||
| *iov = (struct iovec *) realloc(*iov, *iovcnt * sizeof(struct iovec)); |
There was a problem hiding this comment.
We're going to realloc() a lot in edge cases like having a large number of single char args. Tcmalloc will effectively mask some calls but we should cut it to a single realloc() since this is perf sensitive. Maybe leave this as is for the cgroups part, but the env/args part needs a helper to do a single realloc().
userspace/libsinsp/threadinfo.h
Outdated
| struct iovec **iov, int *iovcnt, | ||
| std::string &rem); | ||
|
|
||
| void add_to_iovec(uint32_t &alen, |
There was a problem hiding this comment.
This is the meat of it but I found the calls of it hard to grok. It goes mutable arg, non-mutable args, and then more mutable args. Move alen after include_trailing_null, const include_trailing_null, and personally I like passing alen as a pointer so it's clear that it changes.
There was a problem hiding this comment.
I prefer references, but I did clean things up to be more consistent. I used pointers for methods that allocate and return iovec/iovcnt, and references everywhere else.
| size_t env_len(); | ||
| size_t cgroups_len(); | ||
|
|
||
| void args_to_iovec(struct iovec **iov, int *iovcnt, |
There was a problem hiding this comment.
Add a comment that these functions make shallow copies and put a possibly truncated argument in rem
Also, pretty sure all the *_len() and *_to_iovec() functions and their private calls can be made const
|
Addressed the feedback. I'll squash down before I fully merge the PR, but can you take a look? |
mstemm
force-pushed
the
improve-state-switches
branch
from
632798d to
99e8f93
Compare
Add a new scap_write_proclist_entry_bufs which takes a scap_threadinfo and a set of strings representing the various array-backed strings that can be in a scap_threadinfo struct. In the case of args/env/cgroups, which are represented within sinsp as vectors of strings, allow writing as an iovec, so the vector elements can be written individually without having to be combined first. This is useful when dumping the set of (sinsp) threads, as you can dump a thread without copying all of the strings into the scap_threadinfo struct.
A big cost in creating a new capture file from a live event stream is
dumping the current set of threads to the capture file. This is done
in sinsp_thread_manager::dump_threads_to_file() and involves iterating
over the set of threads and converting each to a scap_threadinfo
struct, multiple times in fact.
To help make that more efficient, we can do a shallow copy of the
strings from the sinsp_threadinfo struct given that the corresponding
scap_threadinfo is temporary.
{args,env,cgroups}_to_scap are replaced by {args,env,cgroups}_to_iovec,
which assembles an iovec containing the strings in the various string
vectors, without actually copying anything in the common case. In the
case of truncation, a "remainder" string is used, which does get a
truncated copy of the last argument/env var/cgroup value that doesn't
fit.
Also, there's now no need to allocate and populate a scap_threadinfo
struct when you're computing the write size for a thread. You can get
all the sizes directly from the sinsp_threadinfo object.
mstemm
force-pushed
the
improve-state-switches
branch
from
99e8f93 to
605d128
Compare
3 participants
This is not ready to merge yet. It was just a way to test the idea of doing shallow copies of threadinfo structs.