Re: DO NOT REPLY [Bug 3884] - SingleThreadModel servlets not pooled results in low performance

[Jon Stevens <jo...@latchkey.com>]

In the bug report 3884, Craig wrote...

> However, instead
> of synchronizing on the entire session object (which is similar to what STM
> does
> on the entire servlet), the internal code locks *only* on the attributes
> collection:
> 
> public void setAttribute(String name, Object value) {
>   ...
>   synchronized (attributes) {
>     ...
>     attributes.put(name, value);
>     ...
>   }
>   ...
> }
>
> so that no other calls to the session that are happening at the same time from
> other threads (like getId() or setMaxInactiveInterval()) are affected by these
> locks.

The attributes 'collection' is a HashMap.

The JDK 1.2 javadocs for HashMap write:

Note that this implementation is not synchronized. If multiple threads
access this map concurrently, and at least one of the threads modifies the
map structurally, it must be synchronized externally. (A structural
modification is any operation that adds or deletes one or more mappings;
merely changing the value associated with a key that an instance already
contains is not a structural modification.) This is typically accomplished
by synchronizing on some object that naturally encapsulates the map.


The last sentence there is the important one that I care about.

Shouldn't it be:

synchronized (this)

????

-jon

Re: DO NOT REPLY [Bug 3884] - SingleThreadModel servlets not pooled results in low performance

["Craig R. McClanahan" <cr...@apache.org>]

On Tue, 2 Oct 2001, Jon Stevens wrote:

> Date: Tue, 02 Oct 2001 10:11:54 -0700
> From: Jon Stevens <jo...@latchkey.com>
> Reply-To: tomcat-dev@jakarta.apache.org
> To: tomcat-dev <to...@jakarta.apache.org>
> Subject: Re: DO NOT REPLY [Bug 3884]  -  SingleThreadModel servlets not
>     pooled results in low performance
>
> In the bug report 3884, Craig wrote...
>
> > However, instead
> > of synchronizing on the entire session object (which is similar to what STM
> > does
> > on the entire servlet), the internal code locks *only* on the attributes
> > collection:
> >
> > public void setAttribute(String name, Object value) {
> >   ...
> >   synchronized (attributes) {
> >     ...
> >     attributes.put(name, value);
> >     ...
> >   }
> >   ...
> > }
> >
> > so that no other calls to the session that are happening at the same time from
> > other threads (like getId() or setMaxInactiveInterval()) are affected by these
> > locks.
>
> The attributes 'collection' is a HashMap.
>
> The JDK 1.2 javadocs for HashMap write:
>
> Note that this implementation is not synchronized. If multiple threads
> access this map concurrently, and at least one of the threads modifies the
> map structurally, it must be synchronized externally. (A structural
> modification is any operation that adds or deletes one or more mappings;
> merely changing the value associated with a key that an instance already
> contains is not a structural modification.) This is typically accomplished
> by synchronizing on some object that naturally encapsulates the map.
>
>
> The last sentence there is the important one that I care about.
>
> Shouldn't it be:
>
> synchronized (this)
>
> ????
>

Using "synchronized(this)" is logically equivalent to adding the
"synchronized" modifier to the method itself.  It would be safe to do this
(as long as you did the same in getAttribute() and setAttribute()), but it
is overkill, because you really only need to protect accesses to the
underlying HashMap itself.  There is no reason to lock down session
methods that don't talk to this HashMap.

If you look at the implementation class
(org.apache.catalina.session.StandardSession), you will see this technique
used to protect access to several different collections:

  attributes - getAttribute(), getAttributeNames(), removeAttrribute(),
               setAttribute(), keys()

  listeners  - addSessionListener(), removeSessionListener(),
               fireSessionEvent(),

  notes      - getNote(), getNoteNames(), removeNote(), setNote(),

In each case, the "synchronized" lock is placed on the underlying
collection.  If we used "synchronized (this)" everywhere, the impact would
be that a call to getAttribute(), for example, would be blocked by a
simultaneous call to fireSessionEvent().  This delay is needless, because
the method implementations never touch the same underlying collections -
so you get better performance by minimizing the scope of what you lock on.

>
> -jon
>

Craig