From 10f1e3259e16e6bf25a25b6a0ae5076945c068ca Mon Sep 17 00:00:00 2001 From: Yoshito Umaoka Date: Mon, 22 Apr 2013 21:04:37 +0000 Subject: [PATCH] ICU-10097 Replace ICURWLock implementation with JDK's ReentrantReadWriteLock. At the same time, disabled the stats collection (never used by ICU library consumers) by default. X-SVN-Rev: 33543 --- .../core/src/com/ibm/icu/impl/ICURWLock.java | 158 ++++-------------- .../core/src/com/ibm/icu/impl/ICUService.java | 8 +- .../ibm/icu/dev/test/util/ICUServiceTest.java | 8 +- .../dev/test/util/ICUServiceThreadTest.java | 6 +- 4 files changed, 43 insertions(+), 137 deletions(-) diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/ICURWLock.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/ICURWLock.java index ec150708983..9f2e79dacb0 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/ICURWLock.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/ICURWLock.java @@ -1,19 +1,23 @@ /** ******************************************************************************* - * Copyright (C) 2001-2006, International Business Machines Corporation and * + * Copyright (C) 2001-2013, International Business Machines Corporation and * * others. All Rights Reserved. * ******************************************************************************* */ package com.ibm.icu.impl; -// See Allan Holub's 1999 column in JavaWorld, and Doug Lea's code for RWLocks with writer preference. +import java.util.concurrent.locks.ReentrantReadWriteLock; /** - *

A simple Reader/Writer lock. This assumes that there will - * be little writing contention. It also doesn't allow - * active readers to acquire and release a write lock, or - * deal with priority inversion issues.

+ *

A Reader/Writer lock originally written for ICU service + * implementation. The internal implementation was replaced + * with the JDK's stock read write lock (ReentrantReadWriteLock) + * for ICU 52.

+ * + *

This assumes that there will be little writing contention. + * It also doesn't allow active readers to acquire and release + * a write lock, or deal with priority inversion issues.

* *

Access to the lock should be enclosed in a try/finally block * in order to ensure that the lock is always released in case of @@ -31,13 +35,9 @@ package com.ibm.icu.impl; * to return statistics on the use of the lock.

*/ public class ICURWLock { - private Object writeLock = new Object(); - private Object readLock = new Object(); - private int wwc; // waiting writers - private int rc; // active readers, -1 if there's an active writer - private int wrc; // waiting readers + private ReentrantReadWriteLock rwl = new ReentrantReadWriteLock(); - private Stats stats = new Stats(); // maybe don't init to start... + private Stats stats = null; /** * Internal class used to gather statistics on the RWLock. @@ -120,84 +120,6 @@ public class ICURWLock { return stats == null ? null : new Stats(stats); } - // utilities - - private synchronized boolean gotRead() { - ++rc; - if (stats != null) { - ++stats._rc; - if (rc > 1) ++stats._mrc; - } - return true; - } - - private synchronized boolean getRead() { - if (rc >= 0 && wwc == 0) { - return gotRead(); - } - ++wrc; - return false; - } - - private synchronized boolean retryRead() { - if (stats != null) ++stats._wrc; - if (rc >= 0 && wwc == 0) { - --wrc; - return gotRead(); - } - return false; - } - - private synchronized boolean finishRead() { - if (rc > 0) { - return (0 == --rc && wwc > 0); - } - throw new IllegalStateException("no current reader to release"); - } - - private synchronized boolean gotWrite() { - rc = -1; - if (stats != null) { - ++stats._wc; - } - return true; - } - - private synchronized boolean getWrite() { - if (rc == 0) { - return gotWrite(); - } - ++wwc; - return false; - } - - private synchronized boolean retryWrite() { - if (stats != null) ++stats._wwc; - if (rc == 0) { - --wwc; - return gotWrite(); - } - return false; - } - - private static final int NOTIFY_NONE = 0; - private static final int NOTIFY_WRITERS = 1; - private static final int NOTIFY_READERS = 2; - - private synchronized int finishWrite() { - if (rc < 0) { - rc = 0; - if (wwc > 0) { - return NOTIFY_WRITERS; - } else if (wrc > 0) { - return NOTIFY_READERS; - } else { - return NOTIFY_NONE; - } - } - throw new IllegalStateException("no current writer to release"); - } - /** *

Acquire a read lock, blocking until a read lock is * available. Multiple readers can concurrently hold the read @@ -209,20 +131,18 @@ public class ICURWLock { * releaseRead when done (for example, in a finally block).

*/ public void acquireRead() { - if (!getRead()) { - for (;;) { - try { - synchronized (readLock) { - readLock.wait(); - } - if (retryRead()) { - return; - } + if (stats != null) { // stats is null by default + synchronized (this) { + stats._rc++; + if (rwl.getReadLockCount() > 0) { + stats._mrc++; } - catch (InterruptedException e) { + if (rwl.isWriteLocked()) { + stats._wrc++; } } } + rwl.readLock().lock(); } /** @@ -234,11 +154,7 @@ public class ICURWLock { * controlled by acquireRead.

*/ public void releaseRead() { - if (finishRead()) { - synchronized (writeLock) { - writeLock.notify(); - } - } + rwl.readLock().unlock(); } /** @@ -253,20 +169,15 @@ public class ICURWLock { * block).

*/ public void acquireWrite() { - if (!getWrite()) { - for (;;) { - try { - synchronized (writeLock) { - writeLock.wait(); - } - if (retryWrite()) { - return; - } - } - catch (InterruptedException e) { + if (stats != null) { // stats is null by default + synchronized (this) { + stats._wc++; + if (rwl.getReadLockCount() > 0 || rwl.isWriteLocked()) { + stats._wwc++; } } } + rwl.writeLock().lock(); } /** @@ -279,19 +190,6 @@ public class ICURWLock { * acquireWrite.

*/ public void releaseWrite() { - switch (finishWrite()) { - case NOTIFY_WRITERS: - synchronized (writeLock) { - writeLock.notify(); - } - break; - case NOTIFY_READERS: - synchronized (readLock) { - readLock.notifyAll(); - } - break; - case NOTIFY_NONE: - break; - } + rwl.writeLock().unlock(); } } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/ICUService.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/ICUService.java index 486a06bfe31..99fc9280bee 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/ICUService.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/ICUService.java @@ -1,6 +1,6 @@ /** ******************************************************************************* - * Copyright (C) 2001-2011, International Business Machines Corporation and * + * Copyright (C) 2001-2013, International Business Machines Corporation and * * others. All Rights Reserved. * ******************************************************************************* */ @@ -959,8 +959,10 @@ public class ICUService extends ICUNotifier { } /** - * Return a string describing the statistics for this service. - * This also resets the statistics. Used for debugging purposes. + * When the statistics for this service is already enabled, + * return the log and resets he statistics. + * When the statistics is not enabled, this method enable + * the statistics. Used for debugging purposes. */ public String stats() { ICURWLock.Stats stats = factoryLock.resetStats(); diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/util/ICUServiceTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/util/ICUServiceTest.java index f50e50c5f97..e6295d535d2 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/util/ICUServiceTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/util/ICUServiceTest.java @@ -1,6 +1,6 @@ /** ******************************************************************************* - * Copyright (C) 2001-2010, International Business Machines Corporation and * + * Copyright (C) 2001-2013, International Business Machines Corporation and * * others. All Rights Reserved. * ******************************************************************************* */ @@ -879,6 +879,8 @@ public class ICUServiceTest extends TestFmwk // ICURWLock ICURWLock rwlock = new ICURWLock(); + rwlock.resetStats(); + rwlock.acquireRead(); rwlock.releaseRead(); @@ -896,7 +898,7 @@ public class ICUServiceTest extends TestFmwk rwlock.releaseRead(); errln("no error thrown"); } - catch (IllegalStateException e) { + catch (Exception e) { logln("OK: " + e.getMessage()); } @@ -904,7 +906,7 @@ public class ICUServiceTest extends TestFmwk rwlock.releaseWrite(); errln("no error thrown"); } - catch (IllegalStateException e) { + catch (Exception e) { logln("OK: " + e.getMessage()); } diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/util/ICUServiceThreadTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/util/ICUServiceThreadTest.java index 161e08d370c..e984dca7ae0 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/util/ICUServiceThreadTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/util/ICUServiceThreadTest.java @@ -1,6 +1,6 @@ /** ******************************************************************************* - * Copyright (C) 2001-2010, International Business Machines Corporation and * + * Copyright (C) 2001-2013, International Business Machines Corporation and * * others. All Rights Reserved. * ******************************************************************************* */ @@ -371,6 +371,7 @@ public class ICUServiceThreadTest extends TestFmwk stableService = new ICULocaleService(); registerFactories(stableService, getFactoryCollection(50)); } + if (PRINTSTATS) stableService.stats(); // Enable the stats collection return stableService; } private ICUService stableService; @@ -413,6 +414,7 @@ public class ICUServiceThreadTest extends TestFmwk // run register/unregister on a service public void Test03_ConcurrentRegUnreg() { ICUService service = new ICULocaleService(); + if (PRINTSTATS) service.stats(); // Enable the stats collection for (int i = 0; i < 5; ++i) { new RegisterFactoryThread("[" + i + "]", service, 0, this).start(); } @@ -425,6 +427,7 @@ public class ICUServiceThreadTest extends TestFmwk public void Test04_WitheringService() { ICUService service = new ICULocaleService(); + if (PRINTSTATS) service.stats(); // Enable the stats collection Collection fc = getFactoryCollection(50); registerFactories(service, fc); @@ -452,6 +455,7 @@ public class ICUServiceThreadTest extends TestFmwk // run for ten seconds public void Test05_ConcurrentEverything() { ICUService service = new ICULocaleService(); + if (PRINTSTATS) service.stats(); // Enable the stats collection new RegisterFactoryThread("", service, 500, this).start();