Propsed changes re: multi-threading and typeSystemImpl

[Marshall Schor <ms...@schor.com>]

In addition to http://issues.apache.org/jira/browse/UIMA-500 which 
reduces lock contention,
there are some other changes needed.

TypeSystemImpl is not thread safe, but is used by many threads.

Because it has the property that it is "updated" by one thread, and then 
is read-only by all threads, it can take advantage of the JSR-133 update 
to Java,
and avoid lots of synch's or "volatile" refs.

JSR-133 is in Java 1.5 and later, but this post indicates the part of 
JSR-133 that changes the semantics of intermixing volatile and 
non-volatile refs to allow use of a "guard" flag (such as, in our case, 
the type system "committed" flag),  been put into 1.4 by Sun: 
http://www.cs.umd.edu/~pugh/java/memoryModel/archive/1888.html

By the way, a great reference for all this stuff is 
http://www.cs.umd.edu/~pugh/java/memoryModel/

In light of this, I would propose:

1) we make use of this to implement a one-time "flushing" of any cached 
values when the type system is committed, using a volatile guard value, 
and insure all other threads that are going to have read-only access to 
the type system do a "get" on the guard value. 

2) Change the impl of getArrayType for array types *that are not already 
defined and committed* to throw an error, or at least log a message that 
this
usage is not type safe.

3) The "SymbolTable" class is a general class, but is only used in the 
TypeSystemImpl for holding the Feature Structure and Type Name maps to 
their int codes.  It tries to be thread save, but isn't quite.  For 
instance, it synch locks "writes" to the hashmap, but doesn't synch lock 
"reads" of the hashmap, which
exposes threads doing reads to not seeing the writes done in another 
thread due to the Java memory model.

Rather than fix this, I would rather remove the synchs from Symbol 
Table, switch the Vector to ArrayList, and make use of the fact that the 
TypeSystemImpl is used by a single thread until committed, and then is 
"read-only" (not updated).

Other opinions?

-Marshall

Re: Propsed changes re: multi-threading and typeSystemImpl

[Thilo Goetz <tw...@gmx.de>]

Marshall Schor wrote:
> In addition to http://issues.apache.org/jira/browse/UIMA-500 which
> reduces lock contention,
> there are some other changes needed.
> 
> TypeSystemImpl is not thread safe, but is used by many threads.
> 
> Because it has the property that it is "updated" by one thread, and then
> is read-only by all threads, it can take advantage of the JSR-133 update
> to Java,
> and avoid lots of synch's or "volatile" refs.
> 
> JSR-133 is in Java 1.5 and later, but this post indicates the part of
> JSR-133 that changes the semantics of intermixing volatile and
> non-volatile refs to allow use of a "guard" flag (such as, in our case,
> the type system "committed" flag),  been put into 1.4 by Sun:
> http://www.cs.umd.edu/~pugh/java/memoryModel/archive/1888.html
> 
> By the way, a great reference for all this stuff is
> http://www.cs.umd.edu/~pugh/java/memoryModel/
> 
> In light of this, I would propose:
> 
> 1) we make use of this to implement a one-time "flushing" of any cached
> values when the type system is committed, using a volatile guard value,
> and insure all other threads that are going to have read-only access to
> the type system do a "get" on the guard value.
> 2) Change the impl of getArrayType for array types *that are not already
> defined and committed* to throw an error, or at least log a message that
> this
> usage is not type safe.
> 
> 3) The "SymbolTable" class is a general class, but is only used in the
> TypeSystemImpl for holding the Feature Structure and Type Name maps to
> their int codes.  It tries to be thread save, but isn't quite.  For
> instance, it synch locks "writes" to the hashmap, but doesn't synch lock
> "reads" of the hashmap, which
> exposes threads doing reads to not seeing the writes done in another
> thread due to the Java memory model.
> 
> Rather than fix this, I would rather remove the synchs from Symbol
> Table, switch the Vector to ArrayList, and make use of the fact that the
> TypeSystemImpl is used by a single thread until committed, and then is
> "read-only" (not updated).
> 
> Other opinions?
> 
> -Marshall
> 

Let's not rush this.  I suggest we get 2.2 out of the door, then do this.
Fix UIMA-500, that seems to be safe enough and gets rid of the performance
problem.  For the rest, let's give everybody time to think about it.

--Thilo