Harmony
  1. Harmony
  2. HARMONY-3090

Create a new table based API for thread library functions

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Classlib
    • Labels:
      None
    • Patch Info:
      Patch Available
    • Estimated Complexity:
      Advanced

      Description

      Both DRLVM and the IBM VME maintain their own implementations of the hythr shared library (not always named hythr). This leads to a situation where the VM is linked against one of the shared libraries and the classlib is linked against another. The internal implementation of hythr assumes a certain amount of coordination between the threading functions. For the most part this does not lead to problems when things aren't co-ordinated. However there have been issues.

      One such issue would involve thread local storage (TLS). The Harmony implementation of TLS uses static variables as the basis for its implementation. Those statics are confined on a shared library basis. Meaning that if a TLS value was being set when the VM called a thread lib function, a call to a thread lib function from the classlib would not see that TLS data. This is due to the VM and classlib using different thread library shared libraries.

      The above is the case when the shared libraries contain the same implementation. A similar scenario arises when the shared libraries for threading do not contain the same implementation.

      The thread library used by the VM, for example, could be relying on some storing values in TLS that will modify the behaviour of other calls to thread library functions. When the classlib calls it's own implementation of thread library functions, the behvaiour of those functions will not be modified by the values set in TLS by the VM. This could lead interesting hard to diagnose failures in the runtime as a whole.

      A solution to these problems would be to create a standard thread API, much like the one created for the port library.

      The VM would become responsible for providing the hythr shared library. The classlib (and other tools that use threading functions) would compile against the API and no-opt implementation of hythr.

      The attached patch is one possible implementation of this idea. It creates a thread library table (HyThreadLibrary) much like the port library table. The thread library is created by the port library during the port library's initialization. The thread library can be obtained from the port library using the new port_get_thread_library() function.

      Also, the VMI has been extended to include a new GetThreadLibrary() function. Alternatively, we could leave the VMI as is and modify the following defines in hythread.h to:
      #define THREAD_ACCESS_FROM_VMI(vmi) HyPortLibrary *privatePortLibraryForThread = (*vmi)->GetPortLibrary(vmi); \
      HyThreadLibrary *privateThreadLibrary = privatePortLibraryForThread ->port_get_thread_library(privatePortLibraryForThread)

      THREAD_ACCESS_FROM_ENV and THREAD_ACCESS_FROM_JAVAVM would need to be modified in a similar way.

      1. harmony-3090-as-an-option.diff
        79 kB
        Mark Hindess
      2. thread_api_harmony_patch_20070206.txt
        314 kB
        Ronald Servant
      3. launcher_02052007_patch.txt
        29 kB
        Ronald Servant
      4. thread_api_harmony_patch_20070130a.txt
        286 kB
        Ronald Servant

        Issue Links

          Activity

          Gavin made changes -
          Link This issue is depended upon by HARMONY-2363 [ HARMONY-2363 ]
          Gavin made changes -
          Link This issue blocks HARMONY-2363 [ HARMONY-2363 ]
          Mark Hindess made changes -
          Link This issue is related to HARMONY-5883 [ HARMONY-5883 ]
          Hide
          Mark Hindess added a comment -

          Salikh, it would be good if you could take a look at the changes I've committed and see what needs to be done for HARMONY-2363. Note that I changed the hythr library name on unix to HYTHR_0.2 (from HYTHR_0.1) to make it obvious when the wrong library is being found.

          Show
          Mark Hindess added a comment - Salikh, it would be good if you could take a look at the changes I've committed and see what needs to be done for HARMONY-2363 . Note that I changed the hythr library name on unix to HYTHR_0.2 (from HYTHR_0.1) to make it obvious when the wrong library is being found.
          Hide
          Mark Hindess added a comment -

          I've applied these changes in r523227 and r523228. The -Dhy.no.thr=true option is needed to apply them. We can remove the option and make this the default when DRLVM and IBM VMEs have been updated.

          Ron, can you check that these changes are completed as you'd expect?

          Note, that the hy.no.thr=true option implies the hy.no.sig=true option since I didn't see much point in migrating the sig code (since I think we agreed that we should eliminate this code and let the VM handle signals).

          Show
          Mark Hindess added a comment - I've applied these changes in r523227 and r523228. The -Dhy.no.thr=true option is needed to apply them. We can remove the option and make this the default when DRLVM and IBM VMEs have been updated. Ron, can you check that these changes are completed as you'd expect? Note, that the hy.no.thr=true option implies the hy.no.sig=true option since I didn't see much point in migrating the sig code (since I think we agreed that we should eliminate this code and let the VM handle signals).
          Mark Hindess made changes -
          Assignee Tim Ellison [ tellison ] Mark Hindess [ hindessm ]
          Hide
          Mark Hindess added a comment -

          Vasily,

          Yes. That's it exactly. I know it's going to take a little time for the IBM VME to be updated (assume the same is true for DRLVM) and I wanted to give this feature a better chance of being integrated. I imagine that the flag will be removed when all VM's have been updated to include a hythr dll with the required interface.

          Note: there is a minor error in the patch - the new thr stub makefile should set the EXPNAME to HYTHR_0.2.

          Show
          Mark Hindess added a comment - Vasily, Yes. That's it exactly. I know it's going to take a little time for the IBM VME to be updated (assume the same is true for DRLVM) and I wanted to give this feature a better chance of being integrated. I imagine that the flag will be removed when all VM's have been updated to include a hythr dll with the required interface. Note: there is a minor error in the patch - the new thr stub makefile should set the EXPNAME to HYTHR_0.2.
          Hide
          Vasily Zakharov added a comment -

          Mark,

          As far as I understand, this your patch overrides all the previous patches for this issue, and it may be applied without disrupting the operations of Classlib/IBM VM/DRL VM.

          However, if classlib is built with -Dhy.no.thr=true flag, the hythr library is rebuilt with the new approach, and both IBM VM and DRL VM would not function until they're patched accrordingly.

          Is my understanding correct?

          Show
          Vasily Zakharov added a comment - Mark, As far as I understand, this your patch overrides all the previous patches for this issue, and it may be applied without disrupting the operations of Classlib/IBM VM/DRL VM. However, if classlib is built with -Dhy.no.thr=true flag, the hythr library is rebuilt with the new approach, and both IBM VM and DRL VM would not function until they're patched accrordingly. Is my understanding correct?
          Mark Hindess made changes -
          Attachment harmony-3090-as-an-option.diff [ 12352636 ]
          Hide
          Mark Hindess added a comment -

          In order to give VM a chance to get this change implemented, I'd like to see this change implemented as an option. So I've updated Ron's patch to work against latest trunk and use a -Dhy.no.thr=true ant flag. To simplify the patch, I moved the thread changes to a new thrstub tree, and I moved the hythread.h include to be picked up from there too. I also fixed a few compile problems on linux - in addition to the one Salikh mentions above.

          Tim, feel free to assign this to me (or keep it if you prefer).

          Show
          Mark Hindess added a comment - In order to give VM a chance to get this change implemented, I'd like to see this change implemented as an option. So I've updated Ron's patch to work against latest trunk and use a -Dhy.no.thr=true ant flag. To simplify the patch, I moved the thread changes to a new thrstub tree, and I moved the hythread.h include to be picked up from there too. I also fixed a few compile problems on linux - in addition to the one Salikh mentions above. Tim, feel free to assign this to me (or keep it if you prefer).
          Hide
          Salikh Zakirov added a comment -

          And a minor gripe on the patch: it is full of ^M (CR) characters, so whoever will commit this patch, please make sure to commit files with correct line endings
          (if committing from Windows, CR are okay as long as svn:eol-style is set to 'native', if committing from Linux, there should be no CRs)

          Show
          Salikh Zakirov added a comment - And a minor gripe on the patch: it is full of ^M (CR) characters, so whoever will commit this patch, please make sure to commit files with correct line endings (if committing from Windows, CR are okay as long as svn:eol-style is set to 'native', if committing from Linux, there should be no CRs)
          Hide
          Salikh Zakirov added a comment -

          Thanks, now I've got it, especially the point that thread functions indeed may be residing not in a hythr.dll, making the whole system much more flexible.
          I am all for committing this patch.

          Show
          Salikh Zakirov added a comment - Thanks, now I've got it, especially the point that thread functions indeed may be residing not in a hythr.dll, making the whole system much more flexible. I am all for committing this patch.
          Hide
          Ronald Servant added a comment -

          re: By the way, what's the point of having thread library function table, if port library itself is still linked statically to it?

          The port library is statically linked to the thread library table creation functions. The implementation of the table functions does not necessarily need to reside in hythr.

          The class library natives will no longer be statically linked against hythr. Depending on the implementation of the VM, it may or may not be statically linked against hythr.

          It would also be possible to have the port library not statically link against hythr if we felt there was value in this arrangement. I, personally, don't see a big win by doing so.

          Show
          Ronald Servant added a comment - re: By the way, what's the point of having thread library function table, if port library itself is still linked statically to it? The port library is statically linked to the thread library table creation functions. The implementation of the table functions does not necessarily need to reside in hythr. The class library natives will no longer be statically linked against hythr. Depending on the implementation of the VM, it may or may not be statically linked against hythr. It would also be possible to have the port library not statically link against hythr if we felt there was value in this arrangement. I, personally, don't see a big win by doing so.
          Hide
          Salikh Zakirov added a comment -

          The following patch on top of launcher_02052007_patch.txt (29 kb) is needed to compile it on Linux.
          — modules/luni/src/main/native/launcher/unix/main_hlp.c
          +++ modules/luni/src/main/native/launcher/unix/main_hlp.c
          @@ -348,7 +348,7 @@ searchSystemPath (char *filename, char **result)
          *

          • @return 0 on success, any other value on failure.
            */
            -UDATA VMCALL
            +int VMCALL
            main_close_port_library (UDATA descriptor)
            {
            return (UDATA) dlclose ((void *)descriptor);
            @@ -363,7 +363,7 @@ main_close_port_library (UDATA descriptor)
            *
          • @note contents of descriptor are undefined on failure.
            */
            -UDATA VMCALL
            +int VMCALL
            main_open_port_library (UDATA * descriptor)
            {
            void *handle;

          By the way, what's the point of having thread library function table, if port library itself is still linked statically to it?
          (And it would be reasonable to expect VM to be linked statically to its thread library as well)

          Show
          Salikh Zakirov added a comment - The following patch on top of launcher_02052007_patch.txt (29 kb) is needed to compile it on Linux. — modules/luni/src/main/native/launcher/unix/main_hlp.c +++ modules/luni/src/main/native/launcher/unix/main_hlp.c @@ -348,7 +348,7 @@ searchSystemPath (char *filename, char **result) * @return 0 on success, any other value on failure. */ -UDATA VMCALL +int VMCALL main_close_port_library (UDATA descriptor) { return (UDATA) dlclose ((void *)descriptor); @@ -363,7 +363,7 @@ main_close_port_library (UDATA descriptor) * @note contents of descriptor are undefined on failure. */ -UDATA VMCALL +int VMCALL main_open_port_library (UDATA * descriptor) { void *handle; By the way, what's the point of having thread library function table, if port library itself is still linked statically to it? (And it would be reasonable to expect VM to be linked statically to its thread library as well)
          Tim Ellison made changes -
          Assignee Tim Ellison [ tellison ]
          Ronald Servant made changes -
          Attachment thread_api_harmony_patch_20070206.txt [ 12350969 ]
          Hide
          Ronald Servant added a comment -

          The patch thread_api_harmony_patch_20070206.txt contains the code changes for creating a new table based thread API much like the port library table based API without any VMI changes.

          The thread library is created by the port library and obtained through the new port library function port_get_thread_library().

          Show
          Ronald Servant added a comment - The patch thread_api_harmony_patch_20070206.txt contains the code changes for creating a new table based thread API much like the port library table based API without any VMI changes. The thread library is created by the port library and obtained through the new port library function port_get_thread_library().
          Salikh Zakirov made changes -
          Link This issue blocks HARMONY-2363 [ HARMONY-2363 ]
          Ronald Servant made changes -
          Attachment launcher_02052007_patch.txt [ 12350391 ]
          Hide
          Ronald Servant added a comment -

          Parse the "-vmdir:" command line option before we create the port library and add the vmdir to the library loading path.

          This lays the groundwork to allow the portlibrary to link against a hythr.dll which would now be a VM specific subdirectory.

          To accomplish this a small set of port library functions have been copied to the launcher. They are:

          • memory allocate/free
          • get executable name
          • open/close shared library (modified to only open the hyprt library)
          • shared library function name lookup
          Show
          Ronald Servant added a comment - Parse the "-vmdir:" command line option before we create the port library and add the vmdir to the library loading path. This lays the groundwork to allow the portlibrary to link against a hythr.dll which would now be a VM specific subdirectory. To accomplish this a small set of port library functions have been copied to the launcher. They are: memory allocate/free get executable name open/close shared library (modified to only open the hyprt library) shared library function name lookup
          Ronald Servant made changes -
          Attachment thread_api_harmony_patch_20070130.txt [ 12349934 ]
          Ronald Servant made changes -
          Attachment thread_api_harmony_patch_20070130a.txt [ 12349946 ]
          Hide
          Ronald Servant added a comment -

          Attached corrected patch file. First patch had extra java.util.concurrent java files.

          Show
          Ronald Servant added a comment - Attached corrected patch file. First patch had extra java.util.concurrent java files.
          Ronald Servant made changes -
          Field Original Value New Value
          Attachment thread_api_harmony_patch_20070130.txt [ 12349934 ]
          Hide
          Ronald Servant added a comment -

          This patch is the one described in the problem description.

          Show
          Ronald Servant added a comment - This patch is the one described in the problem description.
          Ronald Servant created issue -

            People

            • Assignee:
              Mark Hindess
              Reporter:
              Ronald Servant
            • Votes:
              1 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:

                Development