Commons Logging
  1. Commons Logging
  2. LOGGING-37

[logging] LogFactory#getLogFactory should not look for method every time

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.4
    • Fix Version/s: 1.2
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      LogFactory checks for the existence of Thread#getContextClassLoader every time
      #getLogFactory is invoked and does a reflective invocation. This is
      unnecessarily expensive if many Log objects are created. An easy patch is to
      remember the Method object; the lookup happens only once and it will massively
      profit from reflection optimizations after a number of calls (a Java code stub
      is generated by the reflection package).

      Patch:

      419a420,426
      > private static Method GET_CONTEXT_CLASS_LOADER = null;
      > static {
      > try

      { > GET_CONTEXT_CLASS_LOADER = Thread.class.getMethod("getContextClassLoad er", null); > }

      catch (NoSuchMethodException e)

      { > }

      > }
      436,439c443
      < try {
      < // Are we running on a JDK 1.2 or later system?
      < Method method = Thread.class.getMethod("getContextClassLoader", nu
      ll);
      <

      > if(GET_CONTEXT_CLASS_LOADER != null)

      { 442c446 < classLoader = (ClassLoader)method.invoke(Thread.currentThread( ), null); --- > classLoader = (ClassLoader)GET_CONTEXT_CLASS_LOADER.invoke(Thr ead.currentThread(), null); 472c476 < }

      catch (NoSuchMethodException e)

      { --- > }

      else {

        Activity

        Matthias Ernst created issue -
        Hide
        Matthias Ernst added a comment -

        > > private static Method GET_CONTEXT_CLASS_LOADER = null;

        Add a "final" there.

        Show
        Matthias Ernst added a comment - > > private static Method GET_CONTEXT_CLASS_LOADER = null; Add a "final" there.
        Hide
        Simon Kitching added a comment -

        Nice idea, but unfortunately it's not quite that simple.

        The code
        Thread.class.getMethod
        is currently in method directGetContextClassLoader that is invoked from method
        getContextClassLoader via AccessController.doPrivileged.

        The use of SecurityController is for the case where the commons-logging.jar file
        is signed and trusted to use reflection but the calling code is not.

        What happens if we move this call into a static initialisation block of the
        LogFactory class?

        Is there any security on this call, or is the current use of AccessController
        really just meant to wrap the Method.invoke call?

        If we perform this operation from the static initialisation block of the
        LogFactory class, what are the security manager implications? I would guess
        that such an operation would be performed with the privileges of the class being
        loaded, ie would be ok WITHOUT using an AccessController. However I don't know a
        whole lot about this area..

        Show
        Simon Kitching added a comment - Nice idea, but unfortunately it's not quite that simple. The code Thread.class.getMethod is currently in method directGetContextClassLoader that is invoked from method getContextClassLoader via AccessController.doPrivileged. The use of SecurityController is for the case where the commons-logging.jar file is signed and trusted to use reflection but the calling code is not. What happens if we move this call into a static initialisation block of the LogFactory class? Is there any security on this call, or is the current use of AccessController really just meant to wrap the Method.invoke call? If we perform this operation from the static initialisation block of the LogFactory class, what are the security manager implications? I would guess that such an operation would be performed with the privileges of the class being loaded, ie would be ok WITHOUT using an AccessController. However I don't know a whole lot about this area..
        Hide
        Matthias Ernst added a comment -

        That is a valid concern that I didn't think about. I checked the specification
        for #getMethod which would throw a SecurityException in two cases:
        if a security manager would deny

        • member access to Thread.class with scope PUBLIC
        • package access to java.lang

        I would suggest this would be a dysfunctional setup anyway. If you're really
        concerned, wouldn't it be possible to wrap the Method lookup in a privileged
        block as well?

        Show
        Matthias Ernst added a comment - That is a valid concern that I didn't think about. I checked the specification for #getMethod which would throw a SecurityException in two cases: if a security manager would deny member access to Thread.class with scope PUBLIC package access to java.lang I would suggest this would be a dysfunctional setup anyway. If you're really concerned, wouldn't it be possible to wrap the Method lookup in a privileged block as well?
        Hide
        rdonkin@apache.org added a comment -

        After consideration, I'm not happy addressing this for this release.

        I would consider a performance patch with either extensive unit tests
        (preferrably with security managers) or demonstration code (if it's not feasible
        to easily codify into tests) once this release is out. If anyone feels like
        contributing one, please attach to this bugzilla and reopen

        Show
        rdonkin@apache.org added a comment - After consideration, I'm not happy addressing this for this release. I would consider a performance patch with either extensive unit tests (preferrably with security managers) or demonstration code (if it's not feasible to easily codify into tests) once this release is out. If anyone feels like contributing one, please attach to this bugzilla and reopen
        Henri Yandell made changes -
        Field Original Value New Value
        issue.field.bugzillaimportkey 38626 12342905
        Henri Yandell made changes -
        Project Commons [ 12310458 ] Commons Logging [ 12310484 ]
        Key COM-2753 LOGGING-37
        Affects Version/s 1.0.4 [ 12311678 ]
        Assignee Jakarta Commons Developers Mailing List [ commons-dev@jakarta.apache.org ]
        Component/s Logging [ 12311124 ]
        Henri Yandell made changes -
        Affects Version/s 1.0.4 [ 12311713 ]
        Henri Yandell made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Archie Cobbs added a comment -

        I noticed this method consuming inordinate CPU on my system as well.

        Can the next release of commons-logging drop support for Java 1.1? If so then this issue can go away... simply invoke Thread.getContextClassLoader() directly.

        Show
        Archie Cobbs added a comment - I noticed this method consuming inordinate CPU on my system as well. Can the next release of commons-logging drop support for Java 1.1? If so then this issue can go away... simply invoke Thread.getContextClassLoader() directly.
        Thomas Neidhart made changes -
        Resolution Won't Fix [ 2 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Hide
        Thomas Neidhart added a comment -

        I think it is reasonable to make a 1.2 release that drops java 1.1 support.

        Show
        Thomas Neidhart added a comment - I think it is reasonable to make a 1.2 release that drops java 1.1 support.
        Hide
        Sebb added a comment -

        Agreed, but we should only increase the Java version by the minimum necessary.

        Show
        Sebb added a comment - Agreed, but we should only increase the Java version by the minimum necessary.
        Hide
        Thomas Neidhart added a comment -

        Fixed in r1606041.

        Show
        Thomas Neidhart added a comment - Fixed in r1606041.
        Thomas Neidhart made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Fix Version/s 1.2 [ 12327167 ]
        Resolution Fixed [ 1 ]
        Thomas Neidhart made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Matthias Ernst
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development