RFR: 8289710: Move Suspend/Resume classes out of os.hpp

David Holmes dholmes at openjdk.org
Tue Jul 5 13:05:38 UTC 2022


On Mon, 4 Jul 2022 23:07:27 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> Please review this simple change that only renames a few classes and moved some code around. No functional changes.
> 
> The following classes are used only sparingly. They should be moved to a new header file share/runtime/suspend.hpp to minimize the size of os.hpp
> 
> - SuspendedThreadTaskContext
> - SuspendedThreadTask
> - SuspendResume
> 
> I didn't move the OS-specific implementation to a new file -- the POSIX implementation is currently inside [signals_posix.cpp](https://github.com/openjdk/jdk/blob/df063f7db18a40ea7325fe608b3206a6dff812c1/src/hotspot/os/posix/signals_posix.cpp#L1790) mixed with other signal handling code, so it doesn't seem a good idea to move out just the code for the above 3 classes.
> 
> The only other implementation is in os_windows.cpp. I could move the code to suspend_windows.cpp, but I don't feel very motivated unless someone insists.

It is hard for me to see what is truly shared, what is posix-only and what is windows-only. Probably the existing placement is not very good when it comes to making those distinctions, but moving things into new header files just seem to highlight the problem.

I'm also concerned that this and other recent header files changes are breaking the convention that we generally have a foo.hpp and foo.cpp file. In some cases now if I see a declaration in a header file I have to go and search to find the right cpp file.

src/hotspot/share/runtime/suspend.cpp line 34:

> 32: }
> 33: 
> 34: #ifndef _WINDOWS

I'm curious why this isn't needed on windows? With this ifndef this is really posix-only code and so should be in os/posix/suspend_posix.cpp, or at least os_posix.cpp.

-------------

PR: https://git.openjdk.org/jdk/pull/9371


More information about the hotspot-dev mailing list