InternCache - intern() method causing thread synchronization issue

[vadheraju1 ()]

We are using AXIS  and Xfire as our soap stacks which internally use woodstox. and In a load test, we see thread are waiting on InternCache object - due to the fact that intern() method is Synchronized.
We changed the code it seems to be fixed the issue; we need someone from woodstox team to confirm our change.

Jar : wstx-asl-3.0.1
Stack Trace:
at com.ctc.wstx.util.InternCache.intern(InternCache.java:40) - waiting to lock [0x942373a0] (a com.ctc.wstx.util.InternCache) at com.ctc.wstx.sr.NsInputElementStack.resolveAndValidateElement(NsInputElementStack.java:305) at com.ctc.wstx.sr.BasicStreamReader.handleStartElem(BasicStreamReader.java:2826) at com.ctc.wstx.sr.BasicStreamReader.nextFromTree(BasicStreamReader.java:2737) at com.ctc.wstx.sr.BasicStreamReader.next(BasicStreamReader.java:1004) at org.codehaus.xfire.util.stax.DepthXMLStreamReader.next(DepthXMLStreamReader.java:252) at org.codehaus.xfire.util.stax.FragmentStreamReader.next(FragmentStreamReader.java:79) at org.codehaus.xfire.util.jdom.StaxBuilder.buildTree(StaxBuilder.java:466) at org.codehaus.xfire.util.jdom.StaxBuilder.build(StaxBuilder.java:215) at org.codehaus.xfire.soap.handler.ReadHeadersHandler.readHeaders(ReadHeadersHandler.java:134) at org.codehaus.xfire.soap.handler.ReadHeadersHandler.invoke(ReadHeadersHandler.java:59) at org.codehaus.xfire.handler.HandlerPipeline.invoke(HandlerPipeline.java:131) at org.codehaus.xfire.client.Client.onReceive(Client.java:406) at org.codehaus.xfire.transport.http.HttpChannel.sendViaClient(HttpChannel.java:139) at org.codehaus.xfire.transport.http.HttpChannel.send(HttpChannel.java:48) at org.codehaus.xfire.handler.OutMessageSender.invoke(OutMessageSender.java:26) at org.codehaus.xfire.handler.HandlerPipeline.invoke(HandlerPipeline.java:131) at org.codehaus.xfire.client.Invocation.invoke(Invocation.java:79) at org.codehaus.xfire.client.Invocation.invoke(Invocation.java:114) at org.codehaus.xfire.client.Client.invoke(Client.java:336) at org.codehaus.xfire.client.XFireProxy.handleRequest(XFireProxy.java:77) at org.codehaus.xfire.client.XFireProxy.invoke(XFireProxy.java:57)

Class Name and Method Name:
com.ctc.wstx.util.InternCache.intern

Current Implementation:
public synchronized String intern(String input) {
        String result = (String) get(input);
        if (result == null) {
            result = input.intern();
            put(result, result);
        }
        return result;
    }

New implementation:
public String intern(String input) {
        String result = (String) get(input);
        if (result == null) {
            result = putInCache(input);
        }
        return result;
    }
public synchronized String putInCache(String input) {
            String result = input.intern();
            put(result, result);
            return result;
    }

[Cowtowncoder]

On Tue, Oct 6, 2009 at 12:30 PM, vadheraju1 <Rajeswar.Rao@...> wrote:
>
> We are using AXIS  and Xfire as our soap stacks which internally use
> woodstox. and In a load test, we see thread are waiting on InternCache
> object - due to the fact that intern() method is Synchronized.
> We changed the code it seems to be fixed the issue; we need someone from
> woodstox team to confirm our change.
>
> Jar : wstx-asl-3.0.1
...

First of all, thank you for your suggestion. It is always possible
that implementation could be improved.
I'll have a look at proposed change (first create a Jira issue for it.

I have some questions regarding the change. First: version 3.0.1 is
rather old, and although I don't think there have been drastic
changes, it would be good if the performance aspects could be verified
with a more recent version. 3.2.9 should be fully backwards
compatible, so if you could re-check findings using that version it'd
be great.

Also: I am somewhat surprised that this method could be heavily
contested; and that suggests that perhaps your application is not
reusing XMLInputFactory instances as expected. The reason for this is
that intern()ing only occurs if symbol is not found from symbol table;
and symbol table gets populated and recycled on per-input-factory
basis. Which means that fairly quickly such cache misses should be
very rare.
Put another way: this could indicate sub-optimal behavior within
service itself, whereas symbol tables are not being reused. So you may
want to see how reuse is implemented -- it has significant performance
benefits, especially for small to medium size documents.

-+ Tatu +-

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Holger Hoffstätte-4]

vadheraju1 wrote:

> We are using AXIS  and Xfire as our soap stacks which internally use
> woodstox. and In a load test, we see thread are waiting on InternCache
> object - due to the fact that intern() method is Synchronized.
> We changed the code it seems to be fixed the issue; we need someone from
> woodstox team to confirm our change.
>
> New implementation:
> public String intern(String input) {
>         String result = (String) get(input);
>         if (result == null) {
>             result = putInCache(input);
>         }
>         return result;
>     }
> public synchronized String putInCache(String input) {
>             String result = input.intern();
>             put(result, result);
>             return result;
>     }
>

I'm not the woodstox team, but that change is wrong as get()s are now
unsynchronized, which can lead to all sorts of bugs. It is not enough to
synchronize the put().
Unless you need to run on JDK 1.4 it is probably easiest to just use a
ConcurrentMap with proper putIfAbsent(). If you put your InternCache
implementation on the start of the classpath, Woodstox should pick it up.

However, as Tatu has already answered - it is not entirely clear *why*
this method is a bottleneck in your case. Chances are very good that you
will see other (much more severe) bottlenecks come up after this "fix",
especially in Axis (which is horribly broken in itself) and XFire (long
unmaintained).

-h

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Michael Kay]

As a general rule, when you read shared data and then update it, the whole
operation should be done under a synchronization lock: it's not safe to only
get the lock if an update is needed. Having said that, however, an awful lot
of software gets away with it nearly all the time. Certainly, Saxon does! In
practice it depends a little on the design of the data structures that you
use: although it's always unsafe in theory, it's often safe in practice
(i.e. given the design of current JVMs).

Michael Kay
Saxonica  

> -----Original Message-----
> From: vadheraju1 [mailto:Rajeswar.Rao@...]
> Sent: 06 October 2009 20:31
> To: user@...
> Subject: [woodstox-user] InternCache - intern() method
> causing thread synchronization issue
>
>
> We are using AXIS  and Xfire as our soap stacks which
> internally use woodstox. and In a load test, we see thread
> are waiting on InternCache object - due to the fact that
> intern() method is Synchronized.
> We changed the code it seems to be fixed the issue; we need
> someone from woodstox team to confirm our change.
>
> Jar : wstx-asl-3.0.1
> Stack Trace:
> at com.ctc.wstx.util.InternCache.intern(InternCache.java:40)
> - waiting to lock [0x942373a0] (a com.ctc.wstx.util.InternCache) at
> com.ctc.wstx.sr.NsInputElementStack.resolveAndValidateElement(
> NsInputElementStack.java:305)
> at
> com.ctc.wstx.sr.BasicStreamReader.handleStartElem(BasicStreamR
> eader.java:2826)
> at
> com.ctc.wstx.sr.BasicStreamReader.nextFromTree(BasicStreamRead
> er.java:2737)
> at
> com.ctc.wstx.sr.BasicStreamReader.next(BasicStreamReader.java:1004) at
> org.codehaus.xfire.util.stax.DepthXMLStreamReader.next(DepthXM
> LStreamReader.java:252)
> at
> org.codehaus.xfire.util.stax.FragmentStreamReader.next(Fragmen
> tStreamReader.java:79)
> at
> org.codehaus.xfire.util.jdom.StaxBuilder.buildTree(StaxBuilder
> .java:466)
> at
> org.codehaus.xfire.util.jdom.StaxBuilder.build(StaxBuilder.jav
> a:215) at
> org.codehaus.xfire.soap.handler.ReadHeadersHandler.readHeaders
> (ReadHeadersHandler.java:134)
> at
> org.codehaus.xfire.soap.handler.ReadHeadersHandler.invoke(Read
> HeadersHandler.java:59)
> at
> org.codehaus.xfire.handler.HandlerPipeline.invoke(HandlerPipel
> ine.java:131)
> at org.codehaus.xfire.client.Client.onReceive(Client.java:406) at
> org.codehaus.xfire.transport.http.HttpChannel.sendViaClient(Ht
> tpChannel.java:139)
> at
> org.codehaus.xfire.transport.http.HttpChannel.send(HttpChannel
> .java:48)
> at
> org.codehaus.xfire.handler.OutMessageSender.invoke(OutMessageS
> ender.java:26)
> at
> org.codehaus.xfire.handler.HandlerPipeline.invoke(HandlerPipel
> ine.java:131)
> at org.codehaus.xfire.client.Invocation.invoke(Invocation.java:79) at
> org.codehaus.xfire.client.Invocation.invoke(Invocation.java:114) at
> org.codehaus.xfire.client.Client.invoke(Client.java:336) at
> org.codehaus.xfire.client.XFireProxy.handleRequest(XFireProxy.
> java:77) at
> org.codehaus.xfire.client.XFireProxy.invoke(XFireProxy.java:57
> ) at $Proxy2.doCompleteAndTrackForms(Unknown Source) at
> com.metavante.eds.deposit_origination.common.DoBusSvcsDao.invo
> keBusService(DoBusSvcsDao.java:104)
> at
> com.metavante.eds.common.pages.print_forms.PrintFormsSH.doNavi
> gateOff(PrintFormsSH.java:873)
>
> Class Name and Method Name:
> com.ctc.wstx.util.InternCache.intern
>
> Current Implementation:
> public synchronized String intern(String input) {
>         String result = (String) get(input);
>         if (result == null) {
>             result = input.intern();
>             put(result, result);
>         }
>         return result;
>     }
>
> New implementation:
> public String intern(String input) {
>         String result = (String) get(input);
>         if (result == null) {
>             result = putInCache(input);
>         }
>         return result;
>     }
> public synchronized String putInCache(String input) {
>             String result = input.intern();
>             put(result, result);
>             return result;
>     }
>
> --
> View this message in context:
> http://www.nabble.com/InternCache---intern%28%29-method-causin
> g-thread-synchronization-issue-tp25775169p25775169.html
> Sent from the woodstox - user mailing list archive at Nabble.com.
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this list, please visit:
>
>     http://xircles.codehaus.org/manage_email
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email

[Cowtowncoder]

2009/10/6 Holger Hoffstätte <holger.hoffstaette@...>:

> vadheraju1 wrote:
>> We are using AXIS  and Xfire as our soap stacks which internally use
>> woodstox. and In a load test, we see thread are waiting on InternCache
>> object - due to the fact that intern() method is Synchronized.
>> We changed the code it seems to be fixed the issue; we need someone from
>> woodstox team to confirm our change.
>>
>> New implementation:
>> public String intern(String input) {
>>         String result = (String) get(input);
>>         if (result == null) {
>>             result = putInCache(input);
>>         }
>>         return result;
>>     }
>> public synchronized String putInCache(String input) {
>>             String result = input.intern();
>>             put(result, result);
>>             return result;
>>     }
>>
>
> I'm not the woodstox team, but that change is wrong as get()s are now
> unsynchronized, which can lead to all sorts of bugs. It is not enough to
> synchronize the put().

Right: I did mention this in the Jira issue.
It is actually ok to separately sync on get and put(), because of
intervening intern() call guaranteeing that instance is canonical.
More interestingly though, that will just move the bottleneck within
intern() method, which is all but guaranteed to also synchronize with
a global/singleton lock. So in the end I am not quite sure it will be
a net win.
It definitely is not ok to leave sync out of get part, for
unsynchronized containers like HashMap.

> Unless you need to run on JDK 1.4 it is probably easiest to just use a
> ConcurrentMap with proper putIfAbsent(). If you put your InternCache
> implementation on the start of the classpath, Woodstox should pick it up.

Woodstox is 1.4+ for version 4.0, so Concurrent map can not yet be
used, but for 5.0 and above it can be used.
There are quite a few things that JDK 1.5 will add, including
StringBuilder() (unsynced wrt StringBuffer()); and 1.6 would have
nifty array resize/realloc methods.

-+ Tatu +-

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email