openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marc Prud'hommeaux <mprud...@apache.org>
Subject Re: unenhanced class support
Date Thu, 26 Jul 2007 03:05:45 GMT

I'm in favor of committing it. If others have any reservations,  
perhaps we should turn it into a full-blown vote.



On Jul 25, 2007, at 7:18 PM, Patrick Linskey wrote:

>> > 1. One of the big TODOs seems to be support for compound primary  
>> keys
>> > (e.g., implementing
>> > ReflectingPersistenceCapable.pcCopyKeyFieldsToObjectId()).
>>
>> Actually, I think that things might work as-is with compound PKs. We
>
> Nope, it fails.
>
> Ok. So the TCK does not pass with my patch and without enhancement. My
> inclination is to get it committed so that it's in there, and then we
> can work on the TCK failures in parallel. If others agree with this
> strategy, I'll commit it probably sometime tomorrow. I'll make sure
> that whatever I commit passes the TCK with enhancement on, so there
> won't be any regression for people using enhancement. Thoughts?
>
> -Patrick
>
> On 7/25/07, Patrick Linskey <plinskey@gmail.com> wrote:
>> > 1. One of the big TODOs seems to be support for compound primary  
>> keys
>> > (e.g., implementing
>> > ReflectingPersistenceCapable.pcCopyKeyFieldsToObjectId()).
>>
>> Actually, I think that things might work as-is with compound PKs. We
>> definitely need to test, but the ReflectingPC methods only get called
>> in certain contexts, and some of the methods in it are never used,
>> since the subclass versions are used instead. (Subclass instances are
>> put into PCRegistry; ReflectingPC is only used to control instances
>> created by the user.)
>>
>> > 2. Should ImplHelper._unenhancedInstanceMap be some sort of
>> > IdentityMap? If the user overrides MyEntity.equals() to return true
>> > if, say, their names are the same, we could get into trouble by  
>> doing
>> > just a simple map lookup.
>>
>> Nice catch.
>>
>> > 5. I don't completely understand what is happening with the
>> > subclassing and enhancement. When we are redefining the classes, do
>> > we change the methods internally to do something like change String
>> > getName() { return name; } to String getName()
>> > { ((MyGeneratedSubclass) this).pcProvideField(nameFieldIndex); }? I
>> > don't see any issue with doing this, but if it does turn out to  
>> be a
>> > problem, we could alternately have some lookup that does String
>> > getName() { ImplHelper.getPCProxyFor(this).pcProvideField
>> > (nameFieldIndex); }
>>
>> Basically, the subclasses are used whenever OpenJPA loads a type.  
>> What
>> it does depends on a bunch of factors, but the short story is thus:
>>
>> 1. if available, we redefine user-defined methods to call
>> RedefinitionHelper.accessingField() or
>> RedefinitionHelper.settingField(), either using field-access or
>> property-access rules as appropriate.
>>
>> 2. the subclass basically just implements PC, and does some work for
>> serialization and cloning.
>>
>> So the subclasses don't really have proxies -- they actually  
>> implement
>> PersistenceCapable. The only place where we do any proxying /
>> delegating is in ReflectingPC, to support instances created by the
>> user.
>>
>> In both reflecting and redefinition / subclassing, there's a lot of
>> delegating to ImplHelper.getPC(), which will do the map lookup if
>> necessary, but is optimized to support subclass instances, which
>> implement PC, so don't need entries in the map.
>>
>> So, in a field-access redefinition world, the only cost is that
>> associated with maintaining the map of instance => PC for
>> newly-created instances.
>>
>> In a property-access redefinition world, some of the PC methods have
>> additional costs, since they need to reflect to access fields
>> directly, since we've inserted tracking code directly into the
>> superclass. So, some of the PC-related operations will end up using
>> reflection. Additionally, we require that property-access redefined
>> types are implemented with a single backing field per setter/getter
>> pair. We auto-detect the name of the backing field (assuming
>> relatively simple bytecode in the setter / getter pair), so that
>> shouldn't be that burdensome for normal usage. But it does mean that
>> you can't do prop access + redefinition + store everything in a Map
>> internally.
>>
>> -Patrick
>>
>> On 7/25/07, Marc Prud'hommeaux <mprudhom@apache.org> wrote:
>> > Patrick-
>> >
>> > It looks very good to me! A few notes/questions on the patch:
>> >
>> > 1. One of the big TODOs seems to be support for compound primary  
>> keys
>> > (e.g., implementing
>> > ReflectingPersistenceCapable.pcCopyKeyFieldsToObjectId()).
>> >
>> > 2. Should ImplHelper._unenhancedInstanceMap be some sort of
>> > IdentityMap? If the user overrides MyEntity.equals() to return true
>> > if, say, their names are the same, we could get into trouble by  
>> doing
>> > just a simple map lookup.
>> >
>> > 3. It's pretty nice that we only need 54 invocations of
>> > ImplHelper.toPersistenceCapable().
>> >
>> > 4. InstrumentationFactory is scary with all the implementation
>> > dependent assumptions it makes, and the creation of temporary files
>> > and whatnot. If anyone knows of any way to cheat and obtain an
>> > InstrumentationFactory without having to create a jar file with the
>> > appropriately-formatted manifest, it'd be great.
>> >
>> > 5. I don't completely understand what is happening with the
>> > subclassing and enhancement. When we are redefining the classes, do
>> > we change the methods internally to do something like change String
>> > getName() { return name; } to String getName()
>> > { ((MyGeneratedSubclass) this).pcProvideField(nameFieldIndex); }? I
>> > don't see any issue with doing this, but if it does turn out to  
>> be a
>> > problem, we could alternately have some lookup that does String
>> > getName() { ImplHelper.getPCProxyFor(this).pcProvideField
>> > (nameFieldIndex); }
>> >
>> >
>> > Overall, it looks very good! I'm very excited to be able to start
>> > working towards eliminating the enhancement steps that people have
>> > found so cumbersome in the past.
>> >
>> >
>> >
>> > On Jul 24, 2007, at 4:15 PM, Patrick Linskey wrote:
>> >
>> > > Hi,
>> > >
>> > > I attached my current patch to OPENJPA-293. There are a number  
>> of open
>> > > issues in the patch marked by '#####' marks -- I'd appreciate  
>> another
>> > > set of eyes on those items in particular, and on the patch in  
>> general.
>> > >
>> > > Aside from that, I'm planning to run the CTS without  
>> enhancement on at
>> > > some point in the next day or so, and want to get these changes
>> > > committed soon, so that we can start figuring out whether or  
>> not it
>> > > works.
>> > >
>> > > -Patrick
>> > >
>> > > On 7/22/07, Craig L Russell <Craig.Russell@sun.com> wrote:
>> > >> Hi Patrick,
>> > >>
>> > >> On Jul 22, 2007, at 12:51 PM, Patrick Linskey wrote:
>> > >>
>> > >> >> I think that no other implementation will have much of a 

>> better
>> > >> >> solution. So I don't see that we should try to exclude user
>> > >> options
>> > >> >> or a possible solution just because it's a poor performer.
>> > >> >
>> > >> > What about eviction? My feeling is that wherever OpenJPA would
>> > >> > normally clear state (eviction, certain state transitions), we
>> > >> should
>> > >> > keep the state available instead when we can't intercept and
>> > >> reload on
>> > >> > demand.
>> > >>
>> > >> Yes.
>> > >>
>> > >> Craig
>> > >> >
>> > >> > -Patrick
>> > >> >
>> > >> > On 7/21/07, Craig L Russell <Craig.Russell@sun.com> wrote:
>> > >> >>
>> > >> >> On Jul 20, 2007, at 11:42 AM, Patrick Linskey wrote:
>> > >> >>
>> > >> >> > So, I'm looking for answers to the following questions
in
>> > >> >> particular:
>> > >> >> >
>> > >> >> > 1. what should we do about { Java 5, no javaagent, field
>> > >> access }?
>> > >> >> > Should we support this configuration, including the
>> > >> corresponding
>> > >> >> > extra overhead, or should we require either property
 
>> access or a
>> > >> >> > javaagent specified in these configurations?
>> > >> >>
>> > >> >> I think we should do EAGER fetching of fields just like  
>> the other
>> > >> >> implementations have to do.
>> > >> >> >
>> > >> >> > 2. what should we do about { Java 5, no javaagent, property
>> > >> access,
>> > >> >> > flushed | cleared instances }? There is a much lower
 
>> impact to
>> > >> >> doing
>> > >> >> > the dirty tracking in these configurations, since the
 
>> scope is
>> > >> >> > narrower. However, we might also be able to just not
allow
>> > >> flush or
>> > >> >> > clear or multiple sequential transactions if the  
>> persistence
>> > >> >> context
>> > >> >> > has references to unenhanced, unredefined user-created
>> > >> instances.
>> > >> >>
>> > >> >> I think that no other implementation will have much of a 

>> better
>> > >> >> solution. So I don't see that we should try to exclude user
>> > >> options
>> > >> >> or a possible solution just because it's a poor performer.
>> > >> >>
>> > >> >> Craig
>> > >> >>
>> > >> >> Craig Russell
>> > >> >> Architect, Sun Java Enterprise System http://java.sun.com/
>> > >> products/
>> > >> >> jdo
>> > >> >> 408 276-5638 mailto:Craig.Russell@sun.com
>> > >> >> P.S. A good JDO? O, Gasp!
>> > >> >>
>> > >> >>
>> > >> >>
>> > >> >
>> > >> >
>> > >> > --
>> > >> > Patrick Linskey
>> > >> > 202 669 5907
>> > >>
>> > >> Craig Russell
>> > >> Architect, Sun Java Enterprise System http://java.sun.com/ 
>> products/
>> > >> jdo
>> > >> 408 276-5638 mailto:Craig.Russell@sun.com
>> > >> P.S. A good JDO? O, Gasp!
>> > >>
>> > >>
>> > >>
>> > >
>> > >
>> > > --
>> > > Patrick Linskey
>> > > 202 669 5907
>> >
>> >
>>
>>
>> --
>> Patrick Linskey
>> 202 669 5907
>>
>
>
> -- 
> Patrick Linskey
> 202 669 5907


Mime
View raw message