dev@javaserverfaces.java.net

[REVIEW] Issue #66 - Possible optimization regarding execution of PhaseListeners

From: Jacob Hookom <jacob_at_hookom.net>
Date: Mon, 08 Nov 2004 20:48:04 -0600

*Note* New implementation does scope exception catching on a
per-PhaseListener where the previous implementation was scoped on
before/after phase. I would like to propose not catching at all since
the API does not specify any exceptions to be thrown on beforePhase or
afterPhase. Also, all the tests ran successfully with the one change to
TestLifeCycleImpl in leu of changes to exception scoping.

Issue #66

Performance Enhancements to LifeCycleImpl

SECTION: Changes

M src/com/sun/faces/lifecycle/LifecycleImpl.java
- Used ArrayList cloning to prevent lengthy synchronization blocks.
- Exceptions from PhaseListeners will not prevent other pertinent
  listeners from being fired.

M test/com/sun/faces/lifecycle/TestLifecycleImpl.java
- In leu of the changes to exception handling, all registered listeners
  will be called, even if 'b' throws an exception.

SECTION: Diffs

Index: src/com/sun/faces/lifecycle/LifecycleImpl.java
===================================================================
RCS file:
/shared/data/ccvs/repository/javaserverfaces-sources/jsf-ri/src/com/sun/faces/lifecycle/LifecycleImpl.java,v
retrieving revision 1.46
diff -u -r1.46 LifecycleImpl.java
--- src/com/sun/faces/lifecycle/LifecycleImpl.java 29 Oct 2004
19:48:38 -0000 1.46
+++ src/com/sun/faces/lifecycle/LifecycleImpl.java 9 Nov 2004
02:33:30 -0000
@@ -45,7 +45,7 @@
 
 
     // The set of PhaseListeners registered with this Lifecycle instance
- private List listeners = new ArrayList();
+ private ArrayList listeners = new ArrayList();
 
 
     // The set of Phase instances that are executed by the execute() method
@@ -132,21 +132,20 @@
             log.debug("addPhaseListener(" +
listener.getPhaseId().toString()
                       + "," + listener);
         }
- synchronized (listeners) {
- listeners.add(listener);
+ synchronized (this.listeners) {
+ ArrayList temp = (ArrayList) this.listeners.clone();
+ temp.add(listener);
+ this.listeners = temp;
         }
-
     }
 
 
     // Return the set of PhaseListeners that have been registered
     public PhaseListener[] getPhaseListeners() {
-
- synchronized (listeners) {
- PhaseListener results[] = new PhaseListener[listeners.size()];
- return ((PhaseListener[]) listeners.toArray(results));
+ synchronized (this.listeners) {
+ return (PhaseListener[]) this.listeners.toArray(
+ new PhaseListener[this.listeners.size()]);
         }
-
     }
 
 
@@ -163,8 +162,10 @@
                       listener.getPhaseId().toString()
                       + "," + listener);
         }
- synchronized (listeners) {
- listeners.remove(listener);
+ synchronized (this.listeners) {
+ ArrayList temp = (ArrayList) this.listeners.clone();
+ temp.remove(listener);
+ this.listeners = temp;
         }
 
     }
@@ -172,6 +173,11 @@
 
     // ---------------------------------------------------------
Private Methods
 
+ private void executePhase(PhaseId phaseId, Phase phase,
FacesContext context) throws FacesException {
+ if (!skipping(phaseId, context)) {
+ phase.execute(context);
+ }
+ }
 
     // Execute the specified phase, calling all listeners as well
     private void phase(PhaseId phaseId, Phase phase, FacesContext context)
@@ -180,68 +186,68 @@
         if (log.isTraceEnabled()) {
             log.trace("phase(" + phaseId.toString() + "," + context + ")");
         }
-
- int
- i = 0,
- maxBefore = 0;
-
- try {
- // Notify the "beforePhase" method of interested listeners
- // (ascending)
- synchronized (listeners) {
- if (listeners.size() > 0) {
- PhaseEvent event = new PhaseEvent(context, phaseId, this);
- for (i = 0; i < listeners.size(); i++) {
- PhaseListener listener = (PhaseListener) listeners.get(i);
- if (phaseId.equals(listener.getPhaseId()) ||
- PhaseId.ANY_PHASE.equals(listener.getPhaseId())) {
- listener.beforePhase(event);
- }
- maxBefore = i;
- }
- }
- }
- }
- catch (Throwable e) {
- if (log.isTraceEnabled()) {
- log.trace("phase(" + phaseId.toString() + "," + context +
- ") threw exception: " + e + " " + e.getMessage() +
- "\n" + Util.getStackTraceString(e));
- }
- }
-
- try {
- // Execute this phase itself (if still needed)
- if (!skipping(phaseId, context)) {
- phase.execute(context);
- }
- }
- finally {
- try {
- // Notify the "afterPhase" method of interested listeners
- // (descending)
- synchronized (listeners) {
- if (listeners.size() > 0) {
- PhaseEvent event = new PhaseEvent(context, phaseId, this);
- for (i = maxBefore; i >= 0; i--) {
- PhaseListener listener = (PhaseListener) listeners.get(i);
- if (phaseId.equals(listener.getPhaseId()) ||
- PhaseId.ANY_PHASE.equals(listener.getPhaseId())) {
- listener.afterPhase(event);
- }
- }
- }
- }
- }
- catch (Throwable e) {
- if (log.isTraceEnabled()) {
- log.trace("phase(" + phaseId.toString() + "," + context +
- ") threw exception: " + e + " " + e.getMessage() +
- "\n" + Util.getStackTraceString(e));
- }
- }
- }
-
+
+ // grab a pointer to our listeners
+ List ourListeners = this.listeners;
+ int numListeners = ourListeners.size();
+
+ // if we have PhaseListeners to fire
+ if (numListeners > 0) {
+ // instantiate all of our variables
+ PhaseEvent event = new PhaseEvent(context, phaseId, this);
+ PhaseListener listener = null;
+ PhaseId listenerPhaseId = null;
+ int i;
+
+ // call all relevant PhaseListeners before
+ for (i = 0; i < numListeners; i++) {
+ listener = (PhaseListener) ourListeners.get(i);
+ listenerPhaseId = listener.getPhaseId();
+ if (phaseId.equals(listenerPhaseId)
+ || PhaseId.ANY_PHASE.equals(listenerPhaseId)) {
+ try {
+ listener.beforePhase(event);
+ } catch (Throwable e) {
+ if (log.isTraceEnabled()) {
+ log.trace("beforePhase(" +
phaseId.toString() + "," + context +
+ ") threw exception: " + e + " " +
e.getMessage() +
+ "\n" + Util.getStackTraceString(e));
+ }
+ }
+ }
+ }
+ try
+ {
+ // if response is not already handled, execute phase
+ if (!skipping(phaseId, context)) {
+ phase.execute(context);
+ }
+ }
+ finally {
+ // call all relevant PhaseListeners after
+ for (i = numListeners - 1; i >= 0; i--) {
+ listener = (PhaseListener) ourListeners.get(i);
+ listenerPhaseId = listener.getPhaseId();
+ if (phaseId.equals(listenerPhaseId)
+ || PhaseId.ANY_PHASE.equals(listenerPhaseId)) {
+ try {
+ listener.afterPhase(event);
+ } catch (Throwable e) {
+ if (log.isTraceEnabled()) {
+ log.trace("afterPhase(" +
phaseId.toString() + "," + context +
+ ") threw exception: " + e + " "
+ e.getMessage() +
+ "\n" +
Util.getStackTraceString(e));
+ }
+ }
+ }
+ }
+ }
+ } else { // else no PhaseListeners and simply execute the Phase
+ // if response is not already handled, execute phase
+ if (!skipping(phaseId, context)) {
+ phase.execute(context);
+ }
+ }
     }
 
 
Index: test/com/sun/faces/lifecycle/TestLifecycleImpl.java
===================================================================
RCS file:
/shared/data/ccvs/repository/javaserverfaces-sources/jsf-ri/test/com/sun/faces/lifecycle/TestLifecycleImpl.java,v
retrieving revision 1.30
diff -u -r1.30 TestLifecycleImpl.java
--- test/com/sun/faces/lifecycle/TestLifecycleImpl.java 29 Oct 2004
19:48:39 -0000 1.30
+++ test/com/sun/faces/lifecycle/TestLifecycleImpl.java 9 Nov 2004
02:33:32 -0000
@@ -340,10 +340,10 @@
     assertEquals(2,
              phaseCalledA[PhaseId.APPLY_REQUEST_VALUES.getOrdinal()]);
     // verify before for "b" was called, but the after was not
- assertEquals(1,
+ assertEquals(2,
              phaseCalledB[PhaseId.APPLY_REQUEST_VALUES.getOrdinal()]);
     // verify that neither before nor after for "c" were called
- assertEquals(0,
+ assertEquals(2,
              phaseCalledC[PhaseId.APPLY_REQUEST_VALUES.getOrdinal()]);
 
         life.removePhaseListener(a);
@@ -396,7 +396,7 @@
         }
 
     // verify before and after for "a" were called.
- assertEquals(1,
+ assertEquals(2,
              phaseCalledA[PhaseId.APPLY_REQUEST_VALUES.getOrdinal()]);
     // verify before for "b" was called, but the after was not
     assertEquals(2,


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_javaserverfaces.dev.java.net
For additional commands, e-mail: dev-help_at_javaserverfaces.dev.java.net