Mailing-List: contact dev-help@pivot.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@pivot.apache.org
Received-SPF: pass (nike.apache.org: domain of cbartlett.x@gmail.com
 designates 209.85.215.182 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=mime-version:in-reply-to:references:date:message-id:subject:from:to
         :content-type;
        b=X92eFpY5Ish/Qcs7YXcqZ08aedYTzfkyBXzSDf4OXcTK6vBvDaJD1FZpT3vhGdjqS5
         6IeOZEOBChsFuVF2Lsx9B1UMqaI9AQitA2TowivG/Y2XfT+YdoE1+Ip9wQkgqeDqwI71
         hD/HD3w69JSRrCDmsVJOL5fCQQRS1fybtGU/0=
MIME-Version: 1.0
In-Reply-To: <5F7BAB5B-1317-471B-8111-E76EEECB2C3F@verizon.net>
References: <AANLkTin-W3rKCCNYBwm7QZVpx2FceJOVLmn6Uyn=-h3N@mail.gmail.com>
	<5F7BAB5B-1317-471B-8111-E76EEECB2C3F@verizon.net>
Date: Sun, 23 Jan 2011 04:31:02 +0700
Message-ID: <AANLkTimcC2P6POf-1g5-Dj=CaZJjETSgbt9Va8wAB6Yt@mail.gmail.com>
Subject: Re: Some thoughts on BeanAdapter
From: Chris Bartlett <cbartlett.x@gmail.com>
To: dev@pivot.apache.org
Content-Type: multipart/alternative; boundary=90e6ba61556a69d429049a761532

--90e6ba61556a69d429049a761532
Content-Type: text/plain; charset=ISO-8859-1

(I only have a few minutes to reply, so I'll come back with a fuller
response later)

On 23 January 2011 03:19, Greg Brown <gk_brown@verizon.net> wrote:

> > 1. Tooling & Documentation
>
 I'll come back to these points in a 2nd reply tomorrow.


> 2. Performance
> > BeanAdapter currently performs no caching, which is noticeable in
> situations
> > like loading files with CSVSerializer and rendering data driven
> components.
> > These operations might easily involve thousands of repeated calls to
> > BeanAdapter leading to unnecessary CPU usage and poor performance.
>
> Caching could be tough. There's no way to reliably invalidate the cache,
> since we can't enforce that callers only access the bean via BeanDictionary.
> Caching the getter and setter method references is probably a good idea,
> though - that might offer some performance benefit over looking up the
> method each time. Some performance tests would probably shed some light on
> whether or not it would be worth it.
>
> Agreed, I only meant caching the method/field lookup, not data from the
bean itself, but didn't make it explicit.  I was surprised at the speed
increases I saw when testing this as I assumed it would have been optimised
or cached within the JVM.


> > Some caching could be added internally to BeanAdapter instances, but a
> > shared cache of class data (or PivotBeanInfo as suggested above) perhaps
> > scoped at a classloader level, would make instantiating new BeanAdaper
> > instances far less expensive as well as reducing the 'access time' for
> > properties.
>
> Creating a new bean adapter takes almost no time at all. It doesn't perform
> any initialization in the constructor - it just sets a reference to the bean
> object.
>
> The benefit would come from the caching of the accessor methods.  I was
trying to differentiate between a method accessor cache per BeanAdapter, and
one that was shared between BeanAdapters.  A shared cache might well already
contain the required method reference, whereas an internal one would always
start empty.

This is particularly relevant in data driven components where the same bean
property method will need to be looked up and invoked repeatedly.


> > It seems strange to me that BeanAdapter, and specifically reflection, is
> the
> > primary way of changing styles at runtime. I understand the desire to
> > decouple the skin from the component, but for the supplied Pivot skins at
> > least, the properties are known at compile time.
>
> That's true, but themes are pluggable. So styles have to be set dynamically
> - you don't want to compile your app against skin classes that may not be
> there when you actually run your app. That's why it is done via a
> dictionary.
>
> Yep, I understand the need for the Dictionary.  This was just 'background'
for the next section.  I should have said 'need' rather than 'desire' :)

My reference to compile time was to clarify that the skin could implement
Dictionary itself, meaning reflection was not required (assuming the calling
code knows to check whether Dictionary is implemented).


> > It would be trivial (if
> > tedious) to implement Dictionary for the Terra skins either directly or
> via
> > an enclosed & exposed StyleDictionary class. To some, it might seem
> remiss
> > that they don't implement it already.
>
> We actually did it that way originally, and it was a HUGE pain to maintain.
> You end up with a big if block that maps property names to field values. It
> was error prone and kinda ugly. Using BeanAdapter is much cleaner.
>
I thought that might have been the case, and agree that BeanAdapter is a
nice clean way to perform the task, but also that it has performance
implications.

The maintenance side of things is why I mentioned code generation, but it is
not something that I am really keen on either.  I saw it as a way to improve
performance (hopefully) without creating a maintenance nightmare.

Moving the big ugly if blocks into separate StyleDictionary source files
would keep the skin sources clean.


> > Given the simplicity of StyleDictionary, these classes could be
> > automatically generated using data provided by BeanAdapter/PivotBeanInfo
> if
> > desired, but I don't think that would be necessary in this case.
>
> Generating classes or properties is also ugly, unless you add IDE support
> for it, which IMO would be needlessly time-consuming.
>
> I was thinking of just something simple that could be run to regenerate the
source files when required, and would not be part of the regular build.
 Once generated, they would just be checked in to SVN.  When a skin is
changed, its StyleDictionary would be generated again and checked in as a
replacement to the previous one.

BeanAdapter (or any other class) that exposes the 'definitive' list of
get/set methods would just be used to iterate and insert the imports and
content for get() & put() into a standard class template.


> All of the above really boils down to
> a) Create a PivotBeanInfo class or equivalent describing Pivot's take on
 > beans

> Can you provide some concrete examples of what such a class might contain
> that isn't already available via BeanAdapter?
>
> Will do in the next reply.


  > c) Use reflection as a fallback within StyleDictionary
> As I mentioned above, I don't think this is a good idea.
>
> OK.  I'll try to restate my case and include some test code to demonstrate.


One other idea I forgot to include in the original email was to make it
possible to use  custom BeanAdapter and/or StyleDictionary implementations.
 That way if users could easily use alternatives if required.  However, I
suppose that is easy enough to do anyway by modifying the standard Pivot
source and building manually.

Chris

--90e6ba61556a69d429049a761532--