glassfish
  1. glassfish
  2. GLASSFISH-5326

PESessionLockingStandardPipeline causes performance regression

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 9.1.1
    • Fix Version/s: 9.1.1_dev
    • Component/s: web_container
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

    • Issuezilla Id:
      5,326
    • Status Whiteboard:
      Hide

      911Approved

      Show
      911Approved

      Description

      The change to use PESessionLockingStandardPipeline instead of WebPipeline (set
      in the FileBuilderStrategy and MemoryBuilderStrategy classes) has caused a 2-4%
      performance regression in servlet operations (depending on the complexity of the
      servlet). These are on standard servlets/jsps with no failover enabled (I'm not
      sure if that's relevant, but given the strategy classes involved, I thought I'd
      mention it).

        Activity

        Hide
        jluehe added a comment -

        ..

        Show
        jluehe added a comment - ..
        Hide
        jluehe added a comment -

        Hi Scott,

        the PESessionLockingStandardPipeline is responsible for foregound-locking an
        HTTP session on the way in (and for foregound-unlocking it on the way out), so
        that the session manager's reaper (background) thread may not purge a session
        while it is in active (i.e., foregound) use.

        You are right, however, that right now, the reaper thread is required to acquire
        a background lock on a session that is a candidate for purging only in the
        persistent, but not the memory-only case.

        The following diffs will address this issue:

        Index: session/StandardManager.java
        ===================================================================
        RCS file:
        /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardManager.java,v
        retrieving revision 1.14.6.2
        diff -u -r1.14.6.2 StandardManager.java
        — session/StandardManager.java 17 Apr 2008 18:37:20 -0000 1.14.6.2
        +++ session/StandardManager.java 18 Jul 2008 20:11:28 -0000
        @@ -942,7 +942,14 @@

        Session sessions[] = findSessions();
        for (int i = 0; i < sessions.length; i++) {

        • sessions[i].isValid();
          + StandardSession sess = (StandardSession) sessions[i];
          + if (sess.lockBackground())
          Unknown macro: {+ try { + sess.isValid(); + } finally { + sess.unlockBackground(); + }+ }

          }

        Unfortunately, this will not address the performance degradation you have
        measured, but merely provides a justification (if there is any such thing) for it.

        Show
        jluehe added a comment - Hi Scott, the PESessionLockingStandardPipeline is responsible for foregound-locking an HTTP session on the way in (and for foregound-unlocking it on the way out), so that the session manager's reaper (background) thread may not purge a session while it is in active (i.e., foregound) use. You are right, however, that right now, the reaper thread is required to acquire a background lock on a session that is a candidate for purging only in the persistent, but not the memory-only case. The following diffs will address this issue: Index: session/StandardManager.java =================================================================== RCS file: /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardManager.java,v retrieving revision 1.14.6.2 diff -u -r1.14.6.2 StandardManager.java — session/StandardManager.java 17 Apr 2008 18:37:20 -0000 1.14.6.2 +++ session/StandardManager.java 18 Jul 2008 20:11:28 -0000 @@ -942,7 +942,14 @@ Session sessions[] = findSessions(); for (int i = 0; i < sessions.length; i++) { sessions [i] .isValid(); + StandardSession sess = (StandardSession) sessions [i] ; + if (sess.lockBackground()) Unknown macro: {+ try { + sess.isValid(); + } finally { + sess.unlockBackground(); + }+ } } Unfortunately, this will not address the performance degradation you have measured, but merely provides a justification (if there is any such thing) for it.
        Hide
        harpreet added a comment -

        Approved for v2.1.

        Show
        harpreet added a comment - Approved for v2.1.
        Hide
        jluehe added a comment -

        This is fixed by improving the correctness of the existing solution (which
        leverages SessionLock and PESessionLockingStandardPipeline), rather than
        addressing the performance degradation.

        The Servlet spec requires that a session must be kept alive while it is in
        active use, even if it has already expired.

        The original Tomcat code has been using an instance variable "accessCount" (of
        type int) in StandardSession.java to indicates the number of threads
        that are accessing a given session: When a thread acquires a session,
        "accessCount" is incremented, and when the thread is done processing the
        request/response, "accessCount" is decremented.

        While a session's "accessCount" is greater than zero, the reaper thread is
        prevented from purging the session, even if it has expired.

        The original code in Tomcat declared "accessCount" as of type "int", and did not
        synchronize access to it, which has led to thread-safety issues and potential
        memory leaks: In some cases, "accessCount" would never return back to zero,
        causing its session to stay around forever.

        The latest Tomcat codebase has fixed the visibility issues around "accessCount"
        by changing its type from an "int" to an "AtomicInteger", but it avoids any of
        the inherent performance degradation by checking "accessCount" only if running
        in "Servlet spec compliant" mode (yes, such a flag exists in Tomcat), see this
        code in StandardSession,java:

        protected static final boolean ACTIVITY_CHECK =
        Globals.STRICT_SERVLET_COMPLIANCE

        Boolean.valueOf(System.getProperty("org.apache.catalina.session.StandardSession.ACTIVITY_CHECK",
        "false")).booleanValue();

        and

        if (ACTIVITY_CHECK && accessCount.get() > 0)

        { return true; }

        By default, Tomcat runs in non-compliant mode (see
        apache-tomcat-6.0.16-src/java/org/apache/catalina/Globals):

        /**

        • The master flag which controls strict servlet specification
        • compliance.
          */
          public static final boolean STRICT_SERVLET_COMPLIANCE =

        Boolean.valueOf(System.getProperty("org.apache.catalina.STRICT_SERVLET_COMPLIANCE",
        "false")).booleanValue();

        In GlassFish, we no longer have to rely on "accessCount", which is why the diffs
        below get rid of it. Instead, we rely on a session's SessionLock, which allows a
        session to be locked for foreground- and background activity. There was a
        problem in the existing code in that the reaper thread was not required to
        acquire a background-lock on a given session before attempting to purge it. The
        diffs below fix this. Now that foreground- and background-lock requirements are
        correctly implemented (this is what I was referring to at the beginning of this
        update), we are able to get rid of "accessCount".

        We need to compare how our SessionLock compares performance-wise to the
        "accessCount" AtomicInteger in Tomcat, and see if there is room for
        optimization. But even if both approaches were causing the same performance
        degradation, we would still be at a disadvantage compared with Tomcat, because
        Tomcat does not access its "accessCount" AtomicInteger by default, because it
        runs in non-compliant-with-Servlet-spec mode by default. Being the Java EE RI,
        GlassFish cannot afford to provide any such mode.

        Index: StandardManager.java
        ===================================================================
        RCS file:
        /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardManager.java,v
        retrieving revision 1.14.6.2
        diff -u -r1.14.6.2 StandardManager.java
        — StandardManager.java 17 Apr 2008 18:37:20 -0000 1.14.6.2
        +++ StandardManager.java 15 Aug 2008 17:15:37 -0000
        @@ -942,7 +942,14 @@

        Session sessions[] = findSessions();
        for (int i = 0; i < sessions.length; i++) {

        • sessions[i].isValid();
          + StandardSession sess = (StandardSession) sessions[i];
          + if (sess.lockBackground())
          Unknown macro: {+ try { + sess.isValid(); + } finally { + sess.unlockBackground(); + }+ }

          }

        long timeEnd = System.currentTimeMillis();
        Index: StandardSession.java
        ===================================================================
        RCS file:
        /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardSession.java,v
        retrieving revision 1.34.6.12
        diff -u -r1.34.6.12 StandardSession.java
        — StandardSession.java 27 May 2008 19:18:26 -0000 1.34.6.12
        +++ StandardSession.java 15 Aug 2008 17:15:37 -0000
        @@ -333,12 +333,6 @@

        /**

        • * The access count for this session.
        • */
        • protected transient int accessCount = 0;
          -
          -
        • /**
        • The session version, incremented and used by in-memory-replicating
        • session managers
          */
          @@ -677,10 +671,6 @@
          return false;
          }
        • if (accessCount > 0) { - return true; - }

          -
          /* SJSAS 6329289
          if (maxInactiveInterval >= 0)

          { long timeNow = System.currentTimeMillis(); @@ -735,8 +725,6 @@ this.thisAccessedTime = System.currentTimeMillis(); evaluateIfValid(); - - accessCount++; }

        @@ -744,10 +732,7 @@

        • End the access.
          */
          public void endAccess() { - isNew = false; - accessCount--; - }

        cvs commit: Examining .
        Checking in StandardManager.java;
        /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardManager.java,v
        <-- StandardManager.java
        new revision: 1.14.6.3; previous revision: 1.14.6.2
        done
        Checking in StandardSession.java;
        /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardSession.java,v
        <-- StandardSession.java
        new revision: 1.34.6.13; previous revision: 1.34.6.12
        done

        Show
        jluehe added a comment - This is fixed by improving the correctness of the existing solution (which leverages SessionLock and PESessionLockingStandardPipeline), rather than addressing the performance degradation. The Servlet spec requires that a session must be kept alive while it is in active use, even if it has already expired. The original Tomcat code has been using an instance variable "accessCount" (of type int) in StandardSession.java to indicates the number of threads that are accessing a given session: When a thread acquires a session, "accessCount" is incremented, and when the thread is done processing the request/response, "accessCount" is decremented. While a session's "accessCount" is greater than zero, the reaper thread is prevented from purging the session, even if it has expired. The original code in Tomcat declared "accessCount" as of type "int", and did not synchronize access to it, which has led to thread-safety issues and potential memory leaks: In some cases, "accessCount" would never return back to zero, causing its session to stay around forever. The latest Tomcat codebase has fixed the visibility issues around "accessCount" by changing its type from an "int" to an "AtomicInteger", but it avoids any of the inherent performance degradation by checking "accessCount" only if running in "Servlet spec compliant" mode (yes, such a flag exists in Tomcat), see this code in StandardSession,java: protected static final boolean ACTIVITY_CHECK = Globals.STRICT_SERVLET_COMPLIANCE Boolean.valueOf(System.getProperty("org.apache.catalina.session.StandardSession.ACTIVITY_CHECK", "false")).booleanValue(); and if (ACTIVITY_CHECK && accessCount.get() > 0) { return true; } By default, Tomcat runs in non-compliant mode (see apache-tomcat-6.0.16-src/java/org/apache/catalina/Globals): /** The master flag which controls strict servlet specification compliance. */ public static final boolean STRICT_SERVLET_COMPLIANCE = Boolean.valueOf(System.getProperty("org.apache.catalina.STRICT_SERVLET_COMPLIANCE", "false")).booleanValue(); In GlassFish, we no longer have to rely on "accessCount", which is why the diffs below get rid of it. Instead, we rely on a session's SessionLock, which allows a session to be locked for foreground- and background activity. There was a problem in the existing code in that the reaper thread was not required to acquire a background-lock on a given session before attempting to purge it. The diffs below fix this. Now that foreground- and background-lock requirements are correctly implemented (this is what I was referring to at the beginning of this update), we are able to get rid of "accessCount". We need to compare how our SessionLock compares performance-wise to the "accessCount" AtomicInteger in Tomcat, and see if there is room for optimization. But even if both approaches were causing the same performance degradation, we would still be at a disadvantage compared with Tomcat, because Tomcat does not access its "accessCount" AtomicInteger by default, because it runs in non-compliant-with-Servlet-spec mode by default. Being the Java EE RI, GlassFish cannot afford to provide any such mode. Index: StandardManager.java =================================================================== RCS file: /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardManager.java,v retrieving revision 1.14.6.2 diff -u -r1.14.6.2 StandardManager.java — StandardManager.java 17 Apr 2008 18:37:20 -0000 1.14.6.2 +++ StandardManager.java 15 Aug 2008 17:15:37 -0000 @@ -942,7 +942,14 @@ Session sessions[] = findSessions(); for (int i = 0; i < sessions.length; i++) { sessions [i] .isValid(); + StandardSession sess = (StandardSession) sessions [i] ; + if (sess.lockBackground()) Unknown macro: {+ try { + sess.isValid(); + } finally { + sess.unlockBackground(); + }+ } } long timeEnd = System.currentTimeMillis(); Index: StandardSession.java =================================================================== RCS file: /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardSession.java,v retrieving revision 1.34.6.12 diff -u -r1.34.6.12 StandardSession.java — StandardSession.java 27 May 2008 19:18:26 -0000 1.34.6.12 +++ StandardSession.java 15 Aug 2008 17:15:37 -0000 @@ -333,12 +333,6 @@ /** * The access count for this session. */ protected transient int accessCount = 0; - - /** The session version, incremented and used by in-memory-replicating session managers */ @@ -677,10 +671,6 @@ return false; } if (accessCount > 0) { - return true; - } - /* SJSAS 6329289 if (maxInactiveInterval >= 0) { long timeNow = System.currentTimeMillis(); @@ -735,8 +725,6 @@ this.thisAccessedTime = System.currentTimeMillis(); evaluateIfValid(); - - accessCount++; } @@ -744,10 +732,7 @@ End the access. */ public void endAccess() { - isNew = false; - accessCount--; - } cvs commit: Examining . Checking in StandardManager.java; /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardManager.java,v <-- StandardManager.java new revision: 1.14.6.3; previous revision: 1.14.6.2 done Checking in StandardSession.java; /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardSession.java,v <-- StandardSession.java new revision: 1.34.6.13; previous revision: 1.34.6.12 done
        Hide
        jluehe added a comment -

        Incremental fix: Have StandardSession.isValid() return true if the session is
        foreground-locked:

        Checking in StandardSession.java;
        /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardSession.java,v
        <-- StandardSession.java
        new revision: 1.34.6.14; previous revision: 1.34.6.13
        done

        Show
        jluehe added a comment - Incremental fix: Have StandardSession.isValid() return true if the session is foreground-locked: Checking in StandardSession.java; /cvs/glassfish/appserv-webtier/src/java/org/apache/catalina/session/StandardSession.java,v <-- StandardSession.java new revision: 1.34.6.14; previous revision: 1.34.6.13 done
        Hide
        jluehe added a comment -

        Ported fix to V3:

        Sending
        web/web-core/src/main/java/org/apache/catalina/session/StandardManager.java
        Sending
        web/web-core/src/main/java/org/apache/catalina/session/StandardSession.java
        Transmitting file data ..
        Committed revision 21971.

        Show
        jluehe added a comment - Ported fix to V3: Sending web/web-core/src/main/java/org/apache/catalina/session/StandardManager.java Sending web/web-core/src/main/java/org/apache/catalina/session/StandardSession.java Transmitting file data .. Committed revision 21971.

          People

          • Assignee:
            jluehe
            Reporter:
            Scott Oaks
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: