-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new(libsinsp): inspector thread pool #1949
Conversation
bbcf41e
to
32271c5
Compare
Perf diff from master - unit tests
Heap diff from master - unit tests
Heap diff from master - scap file
Benchmarks diff from master
|
32271c5
to
ace2bc4
Compare
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1949 +/- ##
==========================================
- Coverage 74.31% 74.03% -0.28%
==========================================
Files 253 254 +1
Lines 30966 31111 +145
Branches 5403 5417 +14
==========================================
+ Hits 23011 23032 +21
- Misses 7942 8070 +128
+ Partials 13 9 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ace2bc4
to
254e1bd
Compare
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
254e1bd
to
4dba276
Compare
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the plugin API changes but honestly the thread pool scares me. I must be missing some context about why we need it in the first place too (why can't a plugin just start a thread/pool itself? we expect async plugins to do this already).
Exposing this from sinsp feels like encouraging people to do stuff in the thread pool, which is fine, until somebody tries to access tables from there (they're very much not thread safe AFAIK)
So, why do we want this?
For multiple reasons:
|
Do we have a specific use case in mind?
With a fixed number of threads, what happens if you run out of them? It takes just a single routine (that keeps rescheduling itself) to block a thread.
Are we saving anything except a pthread_create call? I suppose passing parameters is easier that way.
Cancelling a thread is far from trivial, especially if the executed code does not cooperate (if it does cooperate, it doesn't matter which thread pool it runs in) One thing I can image this being useful for would be assorted deferred or periodic cleanups (run a lambda in the background whenever we have a moment) in plugins that aren't multithreaded otherwise, but then:
Also, I'm really afraid of pushing multithreading to plugin users before we have a consistent multithreading story ourselves (especially with tables) |
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im> Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
Signed-off-by: Gianmatteo Palmieri <mail@gian.im>
349c5bc
to
d3a2977
Compare
I made a bunch of changes according to the review, most importantly:
WDYT? @jasondellaluce @gnosek |
d3a2977
to
6f13488
Compare
// | ||
// Required: yes if capture_open is defined | ||
// Arguments: | ||
// - s: the plugin state, returned by init(). Can be NULL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing. I assume this is a copy-paste thing? But also, what do we do if ->capture_close returns an error? Is the return value meaningful here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do we do if ->capture_close returns an error? Is the return value meaningful here?
Currently the return value of capture_open
/capture_close
is ignored by the inspector.
But in the future we may need to know if something went wrong in the plugin during capture_open
/capture_close
, we will be able to do that without introducing a breaking change in the plugin api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but what are you going to do about it? It's like a destructor returning an error. I don't think there's any way to handle an error here, except for feeling a little bit sad and moving on (you're not going to cancel the capture stop anyway).
Still, not going to protest too hard about it ;) it just seems useless, not actively harmful
(an error from capture_open
should presumably tear down the whole inspector though, or maybe disable the plugin or something; in any case, it's definitely actionable and should be reported)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left two other minor suggestions! Looks good now!
Signed-off-by: Gianmatteo Palmieri <mail@gian.im> Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com>
6f13488
to
ae81bb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work!
LGTM label has been added. Git tree hash: 3714b104ac9ddd395f24253a87eabc984cae980c
|
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, mrgian The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area libsinsp
/area tests
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Adds a thread pool capable of running non-blocking recurring routines.
Routines can be subscribed both by plugins and the inspector itself.
It also provides a way for plugins to know when the inspector open/closes the event capture.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: