[Mondrian] Improving concurrency on Segment.getCellValue()
Eric McDermid
mcdermid at stonecreek.com
Mon Dec 1 21:17:42 EST 2008
One of my clients uses Mondrian in a situation that requires a fairly
high level of concurrency, and is seeing a slowdown that is at least
partially caused by reader-v-reader contention on
Segment.getCellValue(). That is, because the method itself is
synchronized, multiple readers queue up to go through even though no
writes are occuring.
That seems less than ideal, especially since there's no obvious reason
multiple readers shouldn't be able to execute Segment.getCellValue()
at the same time. I've been doing some analysis in the hopes of
finding a way to allow greater read concurrency with minimal
disruption to existing access patterns, and could use some feedback.
== Overview ==
Segment's method-level synchronization is apparently there to guard
the internal reference to the SegmentDataset ("data") and the
associated segment state ("state", which is either "Loading", "Ready"
or "Failed"). The data reference starts out as null, may be set by
setData(), and should not change again once set. Similarly, state
starts out as Loading, and will eventually be changed to either Ready
(by setData()) or Failed (by setFailIfStillLoading()).
For reference, the synchronized methods of Segment and their
operations on data and state are:
setData()
reads/writes data
reads/writes state
setFailIfStillLoading()
reads data
reads/writes state
getCellValue()
reads data
waitUntilLoaded()
reads state
Additionally, there are several other unsynchronized methods of
Segment that read either data or state:
createSubSegment()
reads data
getData()
reads data
isReady()
reads state
isFailed()
reads state
isLoading()
reads state
(I suspect all of these latter methods really should be guarded.
Though I haven't dug deep enough to be sure their usages aren't
somehow guarded elsewhere, they don't appear to be. The last two in
particular may not present much actual risk, though: isLoading() is
private and only called by waitUntilLoaded(), and isFailed() is only
called from SegmentLoaderTest.)
== Approaches ==
I've been mulling over a couple of ideas to solve the problem:
1. Replace method synchronization in Segment with a CountdownLatch
for data, and a ReentrantReadWriteLock for state.
In this approach, we'd create a CountdownLatch of count 1 when the
segment is created. Each reader would call await() on the latch
before being allowed to access the data reference. Both getData() or
setFailIfStillLoading() would decrement the latch, allowing readers to
proceed. Even though setFailIfStillLoading() doesn't modify data, it
still needs to release any waiting readers if it is to match current
behavior.
Since both setData() and setFailIfStillLoading() can change the value
of state, ReentrantReadWriteLock seems to be the right fit here. Each
reader would be responsible for acquiring the read lock before reading
the value of state, and setData() and setFailIfStillLoading() would
need the write lock before writing.
This approach is fairly simple and encapsulates locking behavior
within a single class, at the cost of requiring some lock overhead for
every read.
2. Delay access to the segment using a synchronized wrapper while it
is loading, then swap out the reference for an unsynchronized
immutable version
Since attempting to read the data reference while it is still loading
is a fairly rare occurrence, another option may be able to split each
Segment object into two objects that share the current Segment
interface: a synchronized wrapper object (let's call its class
"LoadingSegment") and an unsynchronized immutable object that is only
constructed once the underlying SegmentDataset is ready (call its
class "LoadedSegment").
Where we currently create the Segment and add it to Aggregation's
SegmentRefs list, we'd create and add LoadingSegment. Initially,
LoadingSegment would block all calls that can't be completed without
the data reference. Once we are able to construct LoadedSegment, we
pass a reference to it to LoadingSegment, which then allows pending
calls to proceed and delegates their execution to LoadedSegment.
Additionally, we replace the LoadingSegment reference in Aggregation's
list with LoadedSegment, ensuring that any future calls to
getCellValue() incur no synchronization cost.
In LoadedSegment, methods such as setData() and
setFailIfStillLoading() would just raise assertions, while
waitUntilLoaded() would return immediately. Other methods not reliant
on data or state such as wouldContain() would be moved to a common
abstract base class.
== Assessment ==
I see definite advantages to approach #2, particularly in terms of
helping to guarantee the "no modifications once set" behavior that is
currently just implied, as well as in providing lock-free read access
to the underlying data.
Practically speaking, though, I currently lean toward approach #1.
It's simpler to implement (we'd have to use most if not all of
approach #1 to manage locking in LoadingSegment anyway) and allows us
to document/enforce all concurrency expectations and guarantees within
a single class. Initial experiments also suggest it makes a big
difference in the application I'm currently working on.
While #1 may not be as efficient in some very high concurrency
applications as #2, it's unclear just how big a performance difference
there would be. In such a case, I'm inclined to go for the simpler
solution until we see that even this level of concurrency overhead is
actually causing problems.
Does anyone have a distinct preference for one over the other, or
perhaps an alternate approach I should consider?
More information about the Mondrian
mailing list