On Mon, 9 Nov 2020 04:10:30 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due >> to a merge or a rebase. The pull request now contains 64 commits: >> >> - Merge branch '8254162' into 8254231_linker >> - Fix post-merge issues caused by 8219014 >> - Merge branch 'master' into 8254162 >> - Addess remaining feedback from @AlanBateman and @mrserb >> - Address comments from @AlanBateman >> - Fix typo in upcall helper for aarch64 >> - Merge branch '8254162' into 8254231_linker >> - Merge branch 'master' into 8254162 >> - Fix issues with derived buffers and IO operations >> - More 32-bit fixes for TestLayouts >> - ... and 54 more: >> https://git.openjdk.java.net/jdk/compare/a50fdd54...b38afb3f > > A high-level scan through - mostly VM files. I've addressed the open review comments. One of the commits is a bigger change that removes the code duplication in the upcall handler code. The initialization code is moved to the ProgrammableUpcallHandler class' constructor instead. That class is then lazily instantiated using a local `static` variable (see ProgrammableUpcallHandler::instance), which since C++11 guarantees thread-safe initialize-once behaviour. Along with those changes I've also removed some other duplicated code in the native invoker code (ProgrammableInvokerGenerator), cleaned up the includes most files, as well as using a JNI_ENTRY function when doing upcalls (as requested), by splitting the current functionality of the upcall_helper function in 2; one function that does the thread attach, and then other that does the actual upcall (which is the one using JNI_ENTRY). (see: https://github.com/openjdk/jdk/pull/634/commits/719224ca9dc70fce6d28885acfb362fee715ebbd). As discussed, changing ProgrammableUpcallHandler to be a well-known class didn't work, since it is not in java.base. Changes: - Merge both versions of upcall_init and move the code to (the constructor of) ProgrammableUpcallHandler. Using the same lazy singleton pattern as for ForeignGlobals to make initialization thread-safe. - Merge both PorgrammableInvokeGenerator classes into a shared ProgrammableInvoke::Generator class. - Also move ProgrammableStub to ProgrammableInvoke::Stub for better name-spacing - Also move native_invoker_size constant to ProgrammableInvoker (we now have 1 instead of 2) - Merge ProgrammableInvoker::Generator::generate and top-level generate_invoke_native functions (avoiding the need to forward fields) - Split upcall_helper method into ProgrammableUpcallHandler::attach_thread_and_do_upcall and upcall_helper. The former does the thread attach/detach, the latter does the actual upcall. - Add a few comments to ProgrammableUpcallHandler::generate_upcall_stub - Remove unused imports The rest of the review comments were addressed in a set of smaller commits (see timeline on GitHub). The changes therein are: - remove blank line in thread.hpp - Remove os::is_MP() check - remove excessive asserts in ProgrammableInvoker::invoke_native - Extra space after if in jni_util_md (Windows) - Relax ret_addr_offset() assert - Sort includes alphabetically in upcallHandler CPU files - Check result of AttachCurrentThread call - Set copyright year for added files to 2020 (I didn't touch the ARM copyright headers) That should address all open review comments (but please let me know if I've missed something). Thanks for the reviews so far. ------------- PR: https://git.openjdk.java.net/jdk/pull/634
