[
https://issues.apache.org/jira/browse/FELIX-1746?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12765814#action_12765814
]
Jed Wesley-Smith commented on FELIX-1746:
-----------------------------------------
Karl, there are a couple of things that I think are not complete in your patch.
Firstly, the register and unregister methods are still synchronizing on (this)
rather than the m_serviceRegsMap, which increases the chance for that lock to
become contended. Uncontended synchronization is very cheap these days (in
Java6 it boils down to a single CAS per lock acquire for unbiased locking, or
less for unbiased locking if the current thread has already established
ownership of the lock), but contended synchronization can be extremely
expensive, and basically gets more and more so as the number of cores and
threads go up. At a minimum, it is important to use the most specific lock
possible.
Question, you state that HotSpot will conflate the two subsequent lock acquires
into one, can you provide a reference? I have not been able to find one. It is
just as easy to force the matter and surround the two calls in a
synchronized(m_serviceRegsMap).
You also state above that it is difficult to reason about performance with the
COW map solution. This is not actually the case. The COWMap will perform a map
copy under lock during a write, and then modify the copied map. Reads never
lock, they simply read a volatile reference to the underlying effectively
immutable map. The amount of work done in the copy is not a whole lot more than
is done in your read method where the values() collection is copied to an
array. Your map writes are locked as well but must lock while modifying the
value array to ensure atomic updates. Therefore the COWMap disadvantage for
writes is minimal, while reads completely eliminate locking (and therefore the
chance of blocking), which also minimises the number of necessary lock
acquisitions. We have a lot of experience performance testing these structures
and the map has to be pretty large and the ratio of writes to reads very
significant before the write copy under lock becomes significant. For a lot of
usages the COWMap significantly outperforms the java.util.ConcurrentHashMap
(and obviously blows SynchronizedMap out of the water).
You state that Felix in general locks correctly, and I too am very glad that it
does - but this correctness is often implemented by very broad locking. While
this is technically correct, it has the effect of serializing access to a lot
of these classes. It does not take too much load for this to become a major
performance bottleneck.
All that being said, if we can get a slightly modified (as above) commit in for
now and revisit later that seems an acceptable compromise. We will use our
patch on our fork for now and mark it as diverged. Please add the link to the
created issue.
Lastly, it seems some calls to getReference() have been protected by a catch
(IllegalStateException) but not others. Is there a reason for this? Should it
be documented? Why is this change in this changeset at all anyway? Shouldn't
there be a separate issue and commit for this?
> Eliminate contention on ServiceRegistry.getServiceReferences(String, Filter)
> ----------------------------------------------------------------------------
>
> Key: FELIX-1746
> URL: https://issues.apache.org/jira/browse/FELIX-1746
> Project: Felix
> Issue Type: Improvement
> Components: Framework
> Affects Versions: felix-2.0.0
> Reporter: Jed Wesley-Smith
> Assignee: Karl Pauls
> Attachments: blocked-threads.gif.jpg, FELIX-1746-alt.patch,
> FELIX-1746-alt2.patch, FELIX-1746.patch
>
>
> Performance testing has shown that there is significant contention on the
> ServiceRegistry object's monitor during startup. This is caused by Spring DM
> making lots of calls to the synchronized method
> ServiceRegistry.getServiceReferences(String, Filter). This method is
> synchronized in order to protect the m_serviceRegsMap HashMap, but the method
> does a lot more work than simply accessing the map.
> Propose changing the ServiceRegistry to use a thread-safe Map implementation
> that does not require external synchronization, in particular a
> CopyOnWriteMap. I will add a patch that includes a CopyOnWriteMap
> implementation.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
