Haiku: Initial managed libraries support#121880
Haiku: Initial managed libraries support#121880trungnt2910 wants to merge 2 commits intodotnet:mainfrom
Conversation
540d8bb to
f20b31c
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds initial managed libraries support for Haiku OS by implementing the necessary interop and environment code for System.Private.CoreLib. The changes enable basic Haiku platform detection and working set memory queries.
- Adds Haiku OS platform detection with TARGET_HAIKU constant
- Implements WorkingSet property for Haiku using native area_info API
- Provides comprehensive Haiku OS interop definitions (teams, threads, areas, system info)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/OperatingSystem.cs | Adds TARGET_HAIKU constant to platform name detection |
| src/libraries/System.Private.CoreLib/src/System/Environment.Haiku.cs | Implements WorkingSet property for Haiku by iterating process memory areas |
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Configures conditional compilation for Haiku and includes required interop files |
| src/libraries/Common/src/Interop/Haiku/Interop.OS.cs | Defines comprehensive Haiku OS interop structures, enums, and P/Invoke methods for process/thread/memory management |
| src/libraries/Common/src/Interop/Haiku/Interop.Libraries.cs | Defines libroot library constant for Haiku interop |
Comments suppressed due to low confidence (3)
src/libraries/Common/src/Interop/Haiku/Interop.OS.cs:209
- The parameter documentation is incorrect. The
whoparameter is of typeBTeamUsage(an enum specifying self or children), not "The thread ID". It should describe that this parameter specifies whether to get usage information for the team itself or its children.
src/libraries/Common/src/Interop/Haiku/Interop.OS.cs:252 - The parameter documentation should use
<see cref="system_info"/>instead of just "system_info" for consistency with other parameter documentation in this file (e.g., lines 159, 170, 181, 210, 230, 242).
src/libraries/Common/src/Interop/Haiku/Interop.OS.cs:86 - The enum name
thread_stateuses snake_case which is inconsistent with C# naming conventions and the pattern used by other enums in this file (BTeamUsage,BPriority). It should be renamed toThreadStateto follow PascalCase naming convention.
|
C/c @am11 Haiku's |
Each library has its own review team, and they’ll use your tests as a measure of correctness while providing feedback mainly on efficiency and code style. The individual PRs don’t need to build (there’s no Haiku CI here). You could open two or three PRs in parallel if you want to speed up the upstream process (since you already have these patches in your fork tested for a while). |
What I meant was the two share a common file, |
| internal const int B_OS_NAME_LENGTH = 32; | ||
|
|
||
| [LibraryImportAttribute(Interop.Libraries.libroot, SetLastError = false)] | ||
| private static unsafe partial int _get_next_area_info(int team, ref nint cookie, out area_info areaInfo, nuint size); |
There was a problem hiding this comment.
The comment on this in haiku header files says
Is it ok to be depending on "system private" surface here?
There was a problem hiding this comment.
@waddlesplash How likely are the _get_next_[*]_info functions going to break?
There was a problem hiding this comment.
Additionally, is ABI stability (beyond source compatibility) supported on Haiku?
There was a problem hiding this comment.
The alternative is to convert those macros into symbols in System.Native.* then LibraryImport those 🤔
There was a problem hiding this comment.
Additionally, is ABI stability (beyond source compatibility) supported on Haiku?
I am quite sure ABI compatibility within a major release is supported.
There was a problem hiding this comment.
Going by that SystemNative_Sysctl (and many others) pattern, you would create a file called pal_area_info.c or something similar (filename is agnostic of OS) and then #ifdef HAVE_xx where #else case is (void)foo; // unsed for other platforms. This pattern keeps the internal entrypoint accounting same across platforms.
There was a problem hiding this comment.
The issue here would be _get_next_area_info is not the only API I need to pull in.
#121883 pulls in a bunch of other functions in a similar manner. Therefore, I think an OS-guarded file similar to src/native/libs/System.Native/pal_iossupportversion.m would be better?
Furthermore, sysctl is quite among the UNIX-like OSes, despite some differences among the flavors, while _get_next_area_info is Haiku-specific, so #ifdef TARGET_HAIKU would make more sense than #ifdef HAVE_xx.
Finally, with those functions being confirmed as public and ABI stable from the Haiku side, can't we just keep the current implementation and LibraryImport directly?
There was a problem hiding this comment.
Furthermore,
sysctlis quite among the UNIX-like OSes, despite some differences among the flavors, while_get_next_area_infois Haiku-specific, so#ifdef TARGET_HAIKUwould make more sense than#ifdef HAVE_xx.
We are only using sysctl on FreeBSD.
During the port work, try fitting into the most prevalent existing pattern especially when it has given pause to the reviewers and pointed out in docs #121880 (comment).
There was a problem hiding this comment.
@trungnt2910 Are the next steps here clear? I understand it as a consensus around creating pal_area_info.c as that would be most consistent with our existing patterns.
There was a problem hiding this comment.
@jeffhandley No, there is not a consensus at that time.
That said, I have updated the code with what I think is a good compromise:
- We no longer P/Invoke directly, resolving @jkotas's concern.
- The native code is guarded using a Haiku-specific
OS.hheader check, instead of guarding for Haiku itself, resolving @am11's concern. - We keep the current
area_infolayout as a private source-compatible snapshot,AreaInfo, which is passed to and from the native glue function. This resolves my own concern of ABI compatibility. - We make the file
pal_getosinfo.cinstead. This prevents spawning a whole file just forget_next_area_info, since we will need to pull 6-7 otherget_next_[*]_infofunctions in the future.getosinfocaptures both the reliance on theOS.hheader and theget_[*]_infofunction family in Haiku without guarding it specifically for Haiku.
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Show resolved
Hide resolved
|
Still tracking, will take action soon. |
|
Thanks for the reminder, and sorry for the delay. I will take action as soon as I can. |
f20b31c to
265e06b
Compare
265e06b to
acf2867
Compare
|
Local Haiku bootstrap builds are succeeding (after also applying #126701 and making some The CI failures seem like unrelated WASM/Android targets. Most targets are passing. Therefore, I am marking this PR for review again. |
acf2867 to
eef1222
Compare
This contains the code required to build the first managed runtime libraries for Haiku, namely `System.Private.CoreLib`. Co-authored-by: Jessica Hamilton <jessica.l.hamilton@gmail.com>
eef1222 to
54ada29
Compare
| nint cookie = 0; | ||
| long workingSet = 0; | ||
|
|
||
| Interop.OS.AreaInfo areaInfo; | ||
|
|
||
| while (Interop.OS.GetNextAreaInfo(ProcessId, ref cookie, out areaInfo) == 0) | ||
| { | ||
| workingSet += areaInfo.ram_size; | ||
| } |
There was a problem hiding this comment.
Environment.WorkingSet is queried by runtime metrics/event counters and may be polled frequently. This implementation performs a full area enumeration via SystemNative_GetNextAreaInfo on every call (O(number of areas)), which could become noticeably expensive for processes with many mapped regions. If Haiku exposes a direct API for resident/working set size, prefer that; otherwise consider adding a cheaper fast-path or caching within a short time window to reduce syscall/iteration overhead.
There was a problem hiding this comment.
If Haiku exposes a direct API for resident/working set size
@waddlesplash Does Haiku expose such an API? team_info doesn't seem to have such a field.
otherwise consider adding a cheaper fast-path or caching within a short time window to reduce syscall/iteration overhead.
On the library level this may cause unintended stale information. I'd say if any counters want this, it's their job to cache.
This contains the code required to build the first managed runtime libraries for Haiku, namely
System.Private.CoreLib.Part of #55803.