os::get_process_uid and os::rss for BSD - #11
Conversation
This work is sponsored by The FreeBSD Foundation Co-authored-by: Kurt Miller <bsdkurt@gmail.com> Co-authored-by: Greg Lewis <glewis@eyesbeyond.com>
* Add BSD support for RSS process size. This probably needs a tweak for OpenBSD and NetBSD * Sync with Linux
|
👋 Welcome back haraldei! A progress list of the required criteria for merging this PR into |
|
@snake66 This change is no longer ready for integration - check the PR body for details. |
Webrevs
|
This work is sponsored by The FreeBSD Foundation
bsdkurt
left a comment
There was a problem hiding this comment.
This approach seems like a reasonable way to support Apple/FreeBSD/OpenBSD/NetBSD in these functions. The alternative would be less readable.
|
/reviewers 2 |
|
@snake66 Harald, could you bump the reviewers to 2? We want an Oracle dev to review as well and I guess as a committer, I can't issue the command. |
|
/reviewers 2 |
|
I'm afraid you are very unlikely to get an Oracle reviewer for this at this time. |
|
@dholmes-ora Ok, what do you suggest? Just wait for less busy times, or is there a fundamental problem? |
tstuefe
left a comment
There was a problem hiding this comment.
Preface:
- you really should consider splitting bsd from apple in hotspot
- I'd consider adding utility features for BSD, eg BSD_ONLY/NOT_BSD (see examples in macros.hpp) and maybe also for the variants eg FREEBSD_ONLY/NOT_FREEBSD.
| #else | ||
| u_int namelen = 4; | ||
| int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PID, pid}; | ||
| #endif |
There was a problem hiding this comment.
Make namelen const, move it out of the ifdefs, and calculated it based on the array like this
const int namelen = sizeof(mib)/sizeof(mib[0]);
also mib can probably be constexpr as well.
There was a problem hiding this comment.
Updated, but a few comments:
I could not use the NOT_FREEBSD() macro as it would require it to be a variadic macro. That works and is doable, but since the other similar macros are not variadic, I though it best to keep to the same pattern. Still reduced to a single conditional using plain #ifndef makes the code nicer I think.
mib can not be constexpr since it contains the pid which is by definition not const :)
| #else | ||
| pid_t pid = getpid(); | ||
| struct KINFO_PROC_T kp; | ||
| size_t bufSize = sizeof kp; |
There was a problem hiding this comment.
Fixed in latest commit.
| #define KI_RSS p_vm_rssize | ||
| #define KI_UID p_uid | ||
| #define KI_PID p_pid | ||
| #endif |
There was a problem hiding this comment.
This is an eyesore. Can this be shortened, and/or moved into a helper include?
Less busy time may help (JEP-401 and JEP-539 are our priority on VM side right now) and we also have a lot of folk on summer vacations. Not many people with experience across different BSD flavors either (I don't have it). |
|
@tstuefe Thanks a lot for the feedback!
That would be our preference too. But I think we need to have someone from the apple side join us in that effort. From our perspective, I think it would be fine if everything that is under "bsd" now is renamed/moved to "macos" or something, and then we'll deal with the fallout on our side.
Yeah that's a good point. I'll see what that will do for the code. |
|
@dholmes-ora Yeah, I see those JEP's are quite involved, and touches a lot of code! And no worries, we'll wait for our time. |
Why? Just do it :-) The code for MacOS would not be affected, and we'd be less worried that a change in BSD accidentally breaks MacOS.
Sure. os/macos would be the obvious choice. |
Ok! I'll go for it, and see how it's received :) |
Redused the amount of conditional sections to only concern the part of the mib that is different between the platforms. This work is sponsored by The FreeBSD Foundation
This work is sponsored by The FreeBSD Foundation
|
Since I need one more review anyways, I thought I'd just as well submit another related refactoring to these functions. |
|
That didn't seem too successfull... I'll have a look at it tomorrow. |
This work is sponsored by The FreeBSD Foundation
|
@snake66 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
Port of
os::get_process_uid()andos::rss()to BSD.Also makes sure relevant platform dependent includes are only included where supported.
This work is sponsored by The FreeBSD Foundation
Progress
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/bsd-port.git pull/11/head:pull/11$ git checkout pull/11Update a local copy of the PR:
$ git checkout pull/11$ git pull https://git.openjdk.org/bsd-port.git pull/11/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11View PR using the GUI difftool:
$ git pr show -t 11Using diff file
Download this PR as a diff file:
https://git.openjdk.org/bsd-port/pull/11.diff
Using Webrev
Link to Webrev Comment