FYI: AccessController speedup

FYI: AccessController speedup

[Gary Benson]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

Hi all,

Classpath's VMAccessController uses ThreadLocal objects to store per-
thread state information.  Both Classpath and GCJ have a pure Java
ThreadLocal implementation which results in a lot of extra GC activity
with a security manager.  This commit changes VMAccessController to
use an instance variable in Thread to store its state.  On my Tomcat
benchmark this improves performace from 2400 to 2700 requests per
second.

Cheers,
Gary

[-- Attachment #2: accesscontrolstate.patch --]
[-- Type: text/plain, Size: 14153 bytes --]

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 116080)
+++ ChangeLog	(working copy)
@@ -1,3 +1,14 @@
+2006-08-14  Gary Benson  <gbenson@redhat.com>
+
+	* java/lang/Thread.java (accessControlState): New field.
+	* java/security/VMAccessControlState.java: New file.
+	* java/security/natVMAccessControlState.cc: Likewise.
+	* java/security/VMAccessController.java
+	(contexts, inGetContext): Removed.
+	(pushContext, popContext, getContext): Use VMAccessControlState.
+	* Makefile.am (nat_source_files): Updated.
+	* sources.am, Makefile.in: Rebuilt.
+
 2006-08-10  Gary Benson  <gbenson@redhat.com>
 
 	* include/java-stack.h (GetAccessControlStack): Change return
Index: java/lang/Thread.java
===================================================================
--- java/lang/Thread.java	(revision 116080)
+++ java/lang/Thread.java	(working copy)
@@ -144,6 +144,11 @@
   /** The uncaught exception handler.  */
   UncaughtExceptionHandler exceptionHandler;
 
+  /** The access control state for this thread.  Package accessible
+    * for use by java.security.VMAccessControlState's native method.
+    */
+  Object accessControlState = null;
+  
   // This describes the top-most interpreter frame for this thread.
   RawData interp_frame;
 
Index: java/security/VMAccessControlState.java
===================================================================
--- java/security/VMAccessControlState.java	(revision 0)
+++ java/security/VMAccessControlState.java	(revision 0)
@@ -0,0 +1,103 @@
+/* VMAccessControlState.java -- per-thread state for the access controller.
+   Copyright (C) 2006 Free Software Foundation, Inc.
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2, or (at your option)
+any later version.
+
+This program is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program; see the file COPYING.  If not, write to the
+Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+02110-1301 USA.
+
+Linking this library statically or dynamically with other modules is
+making a combined work based on this library.  Thus, the terms and
+conditions of the GNU General Public License cover the whole
+combination.
+
+As a special exception, the copyright holders of this library give you
+permission to link this library with independent modules to produce an
+executable, regardless of the license terms of these independent
+modules, and to copy and distribute the resulting executable under
+terms of your choice, provided that you also meet, for each linked
+independent module, the terms and conditions of the license of that
+module.  An independent module is a module which is not derived from
+or based on this library.  If you modify this library, you may extend
+this exception to your version of the library, but you are not
+obligated to do so.  If you do not wish to do so, delete this
+exception statement from your version. */
+
+
+package java.security;
+
+import java.util.LinkedList;
+
+class VMAccessControlState
+{
+  /**
+   * A list of {@link AccessControlContext} objects (which can be
+   * null) for each call to {@link AccessController#doPrivileged()} in
+   * the thread's call stack.
+   */
+  private LinkedList contexts = new LinkedList();
+
+  /**
+   * A flag indicating that we are within a call to {@link
+   * VMAccessController#getContext()}.
+   */
+  private boolean inGetContext = false;
+
+  /**
+   * Not directly instantiable: use getThreadState() instead.
+   */
+  private VMAccessControlState() {}
+
+  /**
+   * Return an object representing the access control state of this
+   * thread.
+   *
+   * @return The access control state of this thread, or
+   * <code>null</code> if the VM is not initialized to the point of
+   * being able to return this.
+   */
+  static native VMAccessControlState getThreadState();
+
+  /**
+   * Indicate whether this thread is within a call to {@link
+   * VMAccessController#getContext()}.
+   *
+   * @return <code>true</code> if this thread is within a call to
+   * {@link VMAccessController#getContext()}.
+   */
+  boolean isInGetContext()
+  {
+    return inGetContext;
+  }
+
+  /**
+   * Specify whether this thread is within a call to {@link
+   * VMAccessController#getContext()}.
+   */
+  void setInGetContext(boolean inGetContext)
+  {
+    this.inGetContext = inGetContext;
+  }
+  
+  /**
+   * Return a list of {@link AccessControlContext} objects (which can
+   * be null) for each call to {@link AccessController#doPrivileged()}
+   * in the thread's call stack.
+   *
+   * @return a list of {@link AccessControlContext} objects.
+   */
+  LinkedList getContexts()
+  {
+    return contexts;
+  }
+}
Index: java/security/natVMAccessControlState.cc
===================================================================
--- java/security/natVMAccessControlState.cc	(revision 0)
+++ java/security/natVMAccessControlState.cc	(revision 0)
@@ -0,0 +1,32 @@
+// natVMAccessControlState.cc -- Native part of the VMAccessControlState class.
+
+/* Copyright (C) 2006 Free Software Foundation, Inc.
+
+   This file is part of libgcj.
+
+This software is copyrighted work licensed under the terms of the
+Libgcj License.  Please consult the file "LIBGCJ_LICENSE" for
+details.  */
+
+#include <config.h>
+
+#include <gcj/cni.h>
+#include <jvm.h>
+
+#include <java/lang/Thread.h>
+#include <java/security/VMAccessControlState.h>
+
+java::security::VMAccessControlState *
+java::security::VMAccessControlState::getThreadState ()
+{
+  java::lang::Thread *thread = java::lang::Thread::currentThread ();
+  if (thread == NULL)
+    return NULL;
+
+  VMAccessControlState *state =
+    reinterpret_cast<VMAccessControlState *> (thread->accessControlState);
+  if (state == NULL)
+    thread->accessControlState = state = new VMAccessControlState ();
+
+  return state;
+}
Index: java/security/VMAccessController.java
===================================================================
--- java/security/VMAccessController.java	(revision 116080)
+++ java/security/VMAccessController.java	(working copy)
@@ -46,21 +46,6 @@
   // -------------------------------------------------------------------------
 
   /**
-   * This is a per-thread stack of AccessControlContext objects (which can
-   * be null) for each call to AccessController.doPrivileged in each thread's
-   * call stack. We use this to remember which context object corresponds to
-   * which call.
-   */
-  private static final ThreadLocal contexts = new ThreadLocal();
-
-  /**
-   * This is a Boolean that, if set, tells getContext that it has already
-   * been called once, allowing us to handle recursive permission checks
-   * caused by methods getContext calls.
-   */
-  private static final ThreadLocal inGetContext = new ThreadLocal();
-
-  /**
    * And we return this all-permissive context to ensure that privileged
    * methods called from getContext succeed.
    */
@@ -103,19 +88,15 @@
    */
   static void pushContext (AccessControlContext acc)
   {
-    if (Thread.currentThread() == null)
+    // Can't really do anything while the VM is initializing.
+    VMAccessControlState state = VMAccessControlState.getThreadState();
+    if (state == null)
       return;
 
     if (DEBUG)
       debug("pushing " + acc);
-    LinkedList stack = (LinkedList) contexts.get();
-    if (stack == null)
-      {
-         if (DEBUG)
-           debug("no stack... creating ");
-        stack = new LinkedList();
-        contexts.set(stack);
-      }
+
+    LinkedList stack = state.getContexts();
     stack.addFirst(acc);
   }
 
@@ -127,7 +108,9 @@
    */
   static void popContext()
   {
-    if (Thread.currentThread() == null)
+    // Can't really do anything while the VM is initializing.
+    VMAccessControlState state = VMAccessControlState.getThreadState();
+    if (state == null)
       return;
 
     if (DEBUG)
@@ -135,12 +118,10 @@
 
     // Stack should never be null, nor should it be empty, if this method
     // and its counterpart has been called properly.
-    LinkedList stack = (LinkedList) contexts.get();
-    if (stack != null)
+    LinkedList stack = state.getContexts();
+    if (!stack.isEmpty())
       {
-        stack.removeFirst();
-        if (stack.isEmpty())
-          contexts.set(null);
+	stack.removeFirst();
       }
     else if (DEBUG)
       {
@@ -159,7 +140,8 @@
   {
     // If the VM is initializing return the all-permissive context
     // so that any security checks succeed.
-    if (Thread.currentThread() == null)
+    VMAccessControlState state = VMAccessControlState.getThreadState();
+    if (state == null)
       return DEFAULT_CONTEXT;
 
     // If we are already in getContext, but called a method that needs
@@ -168,15 +150,14 @@
     //
     // XXX is this necessary? We should verify if there are any calls in
     // the stack below this method that require permission checks.
-    Boolean inCall = (Boolean) inGetContext.get();
-    if (inCall != null && inCall.booleanValue())
+    if (state.isInGetContext())
       {
         if (DEBUG)
           debug("already in getContext");
         return DEFAULT_CONTEXT;
       }
 
-    inGetContext.set(Boolean.TRUE);
+    state.setInGetContext(true);
 
     Object[] stack = getStack();
     Class[] classes = (Class[]) stack[0];
@@ -210,8 +191,8 @@
             // If there was a call to doPrivileged with a supplied context,
             // return that context. If using JAAS doAs*, it should be 
 	    // a context with a SubjectDomainCombiner
-            LinkedList l = (LinkedList) contexts.get();
-            if (l != null)
+            LinkedList l = state.getContexts();
+            if (!l.isEmpty())
               context = (AccessControlContext) l.getFirst();
           }
 
@@ -256,7 +237,7 @@
     else
       context = new AccessControlContext (result);
 
-    inGetContext.set(Boolean.FALSE);
+    state.setInGetContext(false);
     return context;
   }
 
Index: Makefile.am
===================================================================
--- Makefile.am	(revision 116080)
+++ Makefile.am	(working copy)
@@ -829,6 +829,7 @@
 java/nio/channels/natVMChannels.cc \
 java/nio/natDirectByteBufferImpl.cc \
 java/security/natVMAccessController.cc \
+java/security/natVMAccessControlState.cc \
 java/text/natCollator.cc \
 java/util/natResourceBundle.cc \
 java/util/natVMTimeZone.cc \
Index: sources.am
===================================================================
--- sources.am	(revision 116080)
+++ sources.am	(working copy)
@@ -5330,6 +5330,7 @@
 classpath/java/security/Signer.java \
 classpath/java/security/UnrecoverableKeyException.java \
 classpath/java/security/UnresolvedPermission.java \
+java/security/VMAccessControlState.java \
 java/security/VMAccessController.java \
 java/security/VMSecureRandom.java
 
Index: Makefile.in
===================================================================
--- Makefile.in	(revision 116080)
+++ Makefile.in	(working copy)
@@ -293,6 +293,7 @@
 	java/nio/channels/natVMChannels.cc \
 	java/nio/natDirectByteBufferImpl.cc \
 	java/security/natVMAccessController.cc \
+	java/security/natVMAccessControlState.cc \
 	java/text/natCollator.cc java/util/natResourceBundle.cc \
 	java/util/natVMTimeZone.cc java/util/logging/natLogger.cc \
 	java/util/zip/natDeflater.cc java/util/zip/natInflater.cc \
@@ -339,6 +340,7 @@
 	java/nio/channels/natVMChannels.lo \
 	java/nio/natDirectByteBufferImpl.lo \
 	java/security/natVMAccessController.lo \
+	java/security/natVMAccessControlState.lo \
 	java/text/natCollator.lo java/util/natResourceBundle.lo \
 	java/util/natVMTimeZone.lo java/util/logging/natLogger.lo \
 	java/util/zip/natDeflater.lo java/util/zip/natInflater.lo
@@ -4232,6 +4234,7 @@
 classpath/java/security/Signer.java \
 classpath/java/security/UnrecoverableKeyException.java \
 classpath/java/security/UnresolvedPermission.java \
+java/security/VMAccessControlState.java \
 java/security/VMAccessController.java \
 java/security/VMSecureRandom.java
 
@@ -7151,6 +7154,7 @@
 java/nio/channels/natVMChannels.cc \
 java/nio/natDirectByteBufferImpl.cc \
 java/security/natVMAccessController.cc \
+java/security/natVMAccessControlState.cc \
 java/text/natCollator.cc \
 java/util/natResourceBundle.cc \
 java/util/natVMTimeZone.cc \
@@ -7663,6 +7667,9 @@
 	@: > java/security/$(DEPDIR)/$(am__dirstamp)
 java/security/natVMAccessController.lo: java/security/$(am__dirstamp) \
 	java/security/$(DEPDIR)/$(am__dirstamp)
+java/security/natVMAccessControlState.lo:  \
+	java/security/$(am__dirstamp) \
+	java/security/$(DEPDIR)/$(am__dirstamp)
 java/text/$(am__dirstamp):
 	@$(mkdir_p) java/text
 	@: > java/text/$(am__dirstamp)
@@ -7972,6 +7979,8 @@
 	-rm -f java/nio/channels/natVMChannels.lo
 	-rm -f java/nio/natDirectByteBufferImpl.$(OBJEXT)
 	-rm -f java/nio/natDirectByteBufferImpl.lo
+	-rm -f java/security/natVMAccessControlState.$(OBJEXT)
+	-rm -f java/security/natVMAccessControlState.lo
 	-rm -f java/security/natVMAccessController.$(OBJEXT)
 	-rm -f java/security/natVMAccessController.lo
 	-rm -f java/text/natCollator.$(OBJEXT)
@@ -8094,6 +8103,7 @@
 @AMDEP_TRUE@@am__include@ @am__quote@java/net/$(DEPDIR)/natVMNetworkInterface.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@java/nio/$(DEPDIR)/natDirectByteBufferImpl.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@java/nio/channels/$(DEPDIR)/natVMChannels.Plo@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@java/security/$(DEPDIR)/natVMAccessControlState.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@java/security/$(DEPDIR)/natVMAccessController.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@java/text/$(DEPDIR)/natCollator.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@java/util/$(DEPDIR)/natResourceBundle.Plo@am__quote@

Re: FYI: AccessController speedup

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> This commit changes VMAccessController to use an instance
Gary> variable in Thread to store its state.  On my Tomcat benchmark
Gary> this improves performace from 2400 to 2700 requests per second.

Wow.  If shaving cycles here helps... maybe on supported platforms we
should be using real TLS?  Search for HAVE_TLS in natClass.cc to see
how we're already using it.

But maybe it is just avoiding the gratuitous overhead of ThreadLocal
that is a win.

Tom

Re: FYI: AccessController speedup

[Gary Benson]

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> This commit changes VMAccessController to use an instance
> Gary> variable in Thread to store its state.  On my Tomcat benchmark
> Gary> this improves performace from 2400 to 2700 requests per
> Gary> second.
> 
> Wow.  If shaving cycles here helps... maybe on supported platforms
> we should be using real TLS?  Search for HAVE_TLS in natClass.cc to
> see how we're already using it.

Andrew was talking about a way to make it use real TLS by adding stuff
to the compiler.  What kind of priority he gives it will depend if it
starts showing up in profiles I guess.

> But maybe it is just avoiding the gratuitous overhead of ThreadLocal
> that is a win.

It needn't be so slow, but the current implementation is pure Java and
uses hashtables and things.  It leads to loads of hashing and object
creation whenever you do anything with it.

Cheers,
Gary

Re: FYI: AccessController speedup

[Andrew Haley]

Tom Tromey writes:
 > >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
 > 
 > Gary> This commit changes VMAccessController to use an instance
 > Gary> variable in Thread to store its state.  On my Tomcat benchmark
 > Gary> this improves performace from 2400 to 2700 requests per second.
 > 
 > Wow.  If shaving cycles here helps... maybe on supported platforms we
 > should be using real TLS?  Search for HAVE_TLS in natClass.cc to see
 > how we're already using it.
 > 
 > But maybe it is just avoiding the gratuitous overhead of ThreadLocal
 > that is a win.

I'm going to fix gcj to use real TLS for ThreadLocal variables.  I'll
do it this week unless something more urgent comes up.

Andrew.