Hi David, On Mon, Nov 2, 2015 at 9:01 PM, David Holmes <david.hol...@oracle.com> wrote:
> Hi Thomas, > > On 2/11/2015 9:20 PM, Thomas Stüfe wrote: > >> Hi David, >> >> some small changes are needed to make this build and run on AIX. I >> attached a patch file with the needed additions. >> > > Thanks! > > I also checked Linux ppc64, seems to work fine. > I did not run any extensive tests on AIX, so I cannot say for sure if >> this is stable. We (SAP) also may face some problems later when we port >> this to HP-UX, because there, shared libraries using __thread cannot be >> loaded dynamically. >> > > Ouch! > > So, I admit to some small worries, beside the issue with memory leaks on >> older glibc versions. For me, this feels like something which needs >> tight compiler/thread library support from the OS, so it makes us >> vulnerable to running on older systems (older glibc) or building with >> outdated compilers. Therefore it would be nice to have a simple way to >> re-add the pthread-based TLS implementation if needed. >> > > I can't see how to do that without keeping all the existing layers of code > - even though they would be no-ops on all the platforms that support the > compiler-based TLS. Basically just extend what I did for Solaris to the > other platforms. I took a closer look and I now I worry less. I am confident that in case our old platforms experience problemswith __thread, we can reintroduce TLS without too many changes. Just as a test, I changed the AIX implementation from using __thread back to pthread tls just by changing implementations for Thread::current(), Thread::initialize_thread_current() and Thead::clear_thread_current() in thread.cpp. Works fine as expected. Of course this was just a hack, but if we need to go back to pthread tls for AIX or any platform, I think it can be done in a simpler way than before and still be clean. Not terribly important, but I would prefer if Thread::initialize_thread_current() and Thead::clear_thread_current() were not exposed from Thread at all or at least as private as possible. Thread::initialize_thread_current() is called from the OS layer, but Thead::clear_thread_current() is only called from within thread.cpp itself and could be kept at file scope. > > > Apart from that, I like the patch and think the simplification is good >> and worth the effort. >> > > Even if you can't easily add back the pthread-based TLS if needed? > I think we can, if needed. > It is unfortunate that hotspot may still be shackled to the past that way > - we killed off hotspot-express (in part) to remove those shackles and > allow us to modernize the codebase. > > Thanks, > David > > One question about your changes: Before, Thread::current() would assert instead of returning NULL if called before Thread::initialize_thread_current() or after Thead::clear_thread_current() . Now, we just return NULL. Is this intended? Regards, Thomas > Kind Regards, Thomas >> >> >> >> >> >> >> On Mon, Nov 2, 2015 at 7:40 AM, David Holmes <david.hol...@oracle.com >> <mailto:david.hol...@oracle.com>> wrote: >> >> bug: https://bugs.openjdk.java.net/browse/JDK-8132510 >> >> Open webrev: http://cr.openjdk.java.net/~dholmes/8132510/webrev.v2/ >> >> A simple (in principle) but wide-ranging change which should appeal >> to our Code Deletion Engineer's. We implement Thread::current() >> using a compiler/language-based thread-local variable eg: >> >> >> static __thread Thread *_thr_current; >> >> inline Thread* Thread::current() { >> return _thr_current; >> } >> >> with an appropriate setter of course. By doing this we can >> completely remove the platform-specific ThreadLocalStorage >> implementations, and the associated os::thread_local_storage* calls, >> plus all the uses of ThreadLocalStorage::thread() and >> ThreadLocalStorage::get_thread_slow(). This extends the previous >> work done on Solaris to implement ThreadLocalStorage::thread() using >> compiler-based thread-locals. >> >> We can also consolidate nearly all the os_cpu versions of >> MacroAssembler::get_thread on x86 into one cpu specific one ( a >> special variant is still needed for 32-bit Windows). >> >> As a result of this change we have further potential cleanups: >> - all the src/os/<os>/vm/thread_<os>.inline.hpp files are now >> completely empty and could also be removed >> - the MINIMIZE_RAM_USAGE define (which avoids use of the linux >> sp-map "cache" on 32-bit) now has no affect and so could be >> completely removed from the build system >> >> I plan to do the MINIMIZE_RAM_USAGE removal as a follow up CR, but >> could add the removal of the "inline" files to this CR if people >> think it worth removing them. >> >> I have one missing piece on Aarch64 - I need to change >> MacroAssembler::get_thread to simply call Thread::current() as on >> other platforms, but I don't know how to write that. I would >> appreciate it if someone could give me the right code for that. >> >> I would also appreciate comments/testing by the AIX and PPC64 folk >> as well. >> >> A concern about memory-leaks had previously been raised, but >> experiments using simple C code on linux 86 and Solaris showed no >> issues. Also note that Aarch64 already uses this kind of thread-local. >> >> Thanks, >> David >> >> >>
