You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomee.apache.org by Dain Sundstrom <da...@iq80.com> on 2007/01/09 04:59:09 UTC
Removed FastThreadLocal
I removed the FastThreadLocal class which was cruft from way back in
the day when ThreadLocals were very slow. In the process I
generified and clean up all uses of ThreadLocal, but have a few
questions about some of the code. Hopefully someone who can remember
back to early 2000 can help.
On Jan 8, 2007, at 7:50 PM, dain@apache.org wrote:
> ======================================================================
> ========
> --- incubator/openejb/trunk/openejb3/container/openejb-core/src/
> main/java/org/apache/openejb/core/ServerFederation.java (original)
> +++ incubator/openejb/trunk/openejb3/container/openejb-core/src/
> main/java/org/apache/openejb/core/ServerFederation.java Mon Jan 8
> 19:50:03 2007
> @@ -53,17 +53,20 @@
> }
>
> public static void setApplicationServer(ApplicationServer
> server) {
> + // todo why do we restrict null? This makes call to
> setApplicationServer non symetrical. Throw an exception?
> if (server != null) {
> - threadStorage.set(server);
> + applicationServer.set(server);
> }
> }
Any idea why we don't allow null to be set into the application
server? If the thread local holds, null we return localServer.
I think the proper implementation is to make localServer the
initValue for the thread local, and if someone attempts to set the
application server to null, we throw an exception.
> Modified: incubator/openejb/trunk/openejb3/container/openejb-core/
> src/main/java/org/apache/openejb/core/ivm/IntraVmArtifact.java
> URL: http://svn.apache.org/viewvc/incubator/openejb/trunk/openejb3/
> container/openejb-core/src/main/java/org/apache/openejb/core/ivm/
> IntraVmArtifact.java?view=diff&rev=494312&r1=494311&r2=494312
> ======================================================================
> ========
> + // todo why not put in message catalog?
> private static final String NO_ARTIFACT_ERROR = "The artifact
> this object represents could not be found.";
Is there a primer on how to use the message catalog somewhere?
> @@ -59,11 +54,11 @@
> instanceHandle = in.read();
> }
>
> - private Object readResolve() throws ObjectStreamException {
> - List list = (List) thread.get();
> - if (list == null) throw new InvalidObjectException
> (NO_MAP_ERROR);
> + protected Object readResolve() throws ObjectStreamException {
> + List<Object> list = handles.get();
> Object artifact = list.get(instanceHandle);
> if (artifact == null) throw new InvalidObjectException
> (NO_ARTIFACT_ERROR + instanceHandle);
> + // todo WHY?
> if (list.size() == instanceHandle + 1) {
> list.clear();
> }
Does anyone know why we clear the list here? This same code exists in
org.apache.openejb.proxy.ImmutableArtifact. Is this a duplicate class?
Thanks,
-dain
Re: Removed FastThreadLocal
Posted by David Blevins <da...@visi.com>.
On Jan 8, 2007, at 7:59 PM, Dain Sundstrom wrote:
> I removed the FastThreadLocal class which was cruft from way back
> in the day when ThreadLocals were very slow. In the process I
> generified and clean up all uses of ThreadLocal, but have a few
> questions about some of the code. Hopefully someone who can
> remember back to early 2000 can help.
>
> On Jan 8, 2007, at 7:50 PM, dain@apache.org wrote:
>
>> =====================================================================
>> =========
>> --- incubator/openejb/trunk/openejb3/container/openejb-core/src/
>> main/java/org/apache/openejb/core/ServerFederation.java (original)
>> +++ incubator/openejb/trunk/openejb3/container/openejb-core/src/
>> main/java/org/apache/openejb/core/ServerFederation.java Mon Jan 8
>> 19:50:03 2007
>> @@ -53,17 +53,20 @@
>> }
>>
>> public static void setApplicationServer(ApplicationServer
>> server) {
>> + // todo why do we restrict null? This makes call to
>> setApplicationServer non symetrical. Throw an exception?
>> if (server != null) {
>> - threadStorage.set(server);
>> + applicationServer.set(server);
>> }
>> }
>
> Any idea why we don't allow null to be set into the application
> server? If the thread local holds, null we return localServer.
>
> I think the proper implementation is to make localServer the
> initValue for the thread local, and if someone attempts to set the
> application server to null, we throw an exception.
The "if null return the localServer" thing is a situation that's
potentially non-existent. The only situation where I can think it'd
actually occur is if a bean attempted to serialize a Handle or
HomeHandle to disk, but even then I'm not certain. In that
situation, the local server is the one asking for the app server and
could very well do it's own null check and opt to apply it's own
logic if no app server is found.
The other place I imagined it being used, and is definitely not but
should be, is if an app server wanted to swap it's proxies coming off
the wire with local proxies for better performance should those
proxies be used. Even then we just make the private static final a
public static final or better move the static final right into the
IntraVmServer class.
>
>
>> Modified: incubator/openejb/trunk/openejb3/container/openejb-core/
>> src/main/java/org/apache/openejb/core/ivm/IntraVmArtifact.java
>> URL: http://svn.apache.org/viewvc/incubator/openejb/trunk/openejb3/
>> container/openejb-core/src/main/java/org/apache/openejb/core/ivm/
>> IntraVmArtifact.java?view=diff&rev=494312&r1=494311&r2=494312
>> =====================================================================
>> =========
>> + // todo why not put in message catalog?
>> private static final String NO_ARTIFACT_ERROR = "The artifact
>> this object represents could not be found.";
>
> Is there a primer on how to use the message catalog somewhere?
Nowhere. But, ideally each package would have it's own
Messages.properties file. You would then be able to grab your
messages by referencing your package name. Better, we could let
classes pass in MyClass.class and we could figure out their package
name for them... or someday implement a class based strategy and not
have to update any code.
>
>> @@ -59,11 +54,11 @@
>> instanceHandle = in.read();
>> }
>>
>> - private Object readResolve() throws ObjectStreamException {
>> - List list = (List) thread.get();
>> - if (list == null) throw new InvalidObjectException
>> (NO_MAP_ERROR);
>> + protected Object readResolve() throws ObjectStreamException {
>> + List<Object> list = handles.get();
>> Object artifact = list.get(instanceHandle);
>> if (artifact == null) throw new InvalidObjectException
>> (NO_ARTIFACT_ERROR + instanceHandle);
>> + // todo WHY?
>> if (list.size() == instanceHandle + 1) {
>> list.clear();
>> }
>
> Does anyone know why we clear the list here? This same code exists
> in org.apache.openejb.proxy.ImmutableArtifact. Is this a duplicate
> class?
org.apache.openejb.proxy is all duplicate code. It'll get the boot
eventually.
-David