Some thoughts on BeanAdapter

[Chris Bartlett <cb...@gmail.com>]

BeanAdapter is used directly and indirectly in the following notable places
- BXMLSerializer
- CSVSerializer
- JSONSerializer
- StyleDictionary
- ListView.ItemRenderer implementations
- TableView.CellRenderer implementations
- TableViewRowComparator

It works well at the moment, but I believe that better support for
tooling/documentation as well as performance improvements could be provided
with some minor modifications.


1. Tooling & Documentation
There is a lot of useful logic in BeanAdapter and BXMLSerializer which is
hidden away, but could possibly be exposed. Exposing the bean as BeanAdapter
'sees' it with Pivot specifics such as static properties, read only
Sequences & Maps and ListenerLists would make documenting Components and
Skins very easy.

BeanAdapter currently exposes a list of properties (via its PropertyIterator
inner class) but this could be taken further to include the Pivot specifics
as mentioned above, as well as a means to determine how that property will
be accessed (direct field access or via an accessor/mutator method), and
overloaded property setter methods.

Quite understandably, BeanAdapter requires an Object instance. However, some
refactoring could split BeanAdapter into 2 classes, one which
extracts/introspects the class, and one which provides Dictionary access to
instance of the class. Essentially, create a PivotBeanInfo class. This could
easily then be used for providing the data for a Pivot Doclet &
ComponentExplorer.


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.

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.


Various TableView.CellRenderer implementations set styles during the
rendering process. A sizeable TableView with renderers that alter the font,
color or alignment of cells can cause high CPU and very sluggish performance
when simply scrolling the viewport.

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. 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.

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.


One other related point on renderer performance is in regard to the recent
change to use the JSON class. JSON#get(Object, Sequence<String>) first
attempts to get the value via BeanAdapter rather than checking whether the
target class implements Dictionary.


All of the above really boils down to
a) Create a PivotBeanInfo class or equivalent describing Pivot's take on
beans
b) Cache & reuse class data in BeanAdapter
c) Use reflection as a fallback within StyleDictionary
d) Ensure Dictionary implementations are preferred over BeanAdapter
instances (unless caching sufficiently improves BeanAdapter performance)

The majority of the changes could be limited to BeanAdapter allowing testing
merely by swapping in a new implementation with caching


All of the above performance points came to my attention from real world
experience with Pivot, but I know that some people have very strong opinions
on when/if to optimise. I have put together quick tests on my systems and
can really see the benefits from just adding class level caching to
BeanAdapter, and even better using Janino to generate a compiled
getter/setter snippets.


Chris

Re: Some thoughts on BeanAdapter

[Greg Brown <gk...@verizon.net>]

> The map would also need to handle overloaded setters, so would probably be
> class --> property name --> List<Method>, along with a helper method to
> iterate over the List and return the Method matching the required parameter
> types.

Yeah, I was simplifying.  :-)  Just trying to capture the general idea.

>> You can't really do that, though, since the style fields are private to the
>> skin classes themselves.
>> 
> Of course. I forgot about that.  Without wishing to go too far off topic -
> shouldn't they really be protected to allow skins to be more easily
> extended?

Yes, though we tried not to put too much up-front thought into that, since we didn't want to over-design it. It's something that can easily be changed as we discover new use cases, though.

G

Re: Some thoughts on BeanAdapter

[Chris Bartlett <cb...@gmail.com>]

On 23 January 2011 05:07, Greg Brown <gk...@verizon.net> wrote:

> > This is particularly relevant in data driven components where the same
> bean
> > property method will need to be looked up and invoked repeatedly.
>
> Yeah, I think a static map of Class to property name to method might work
> well. We could remove class entries from the map in BeanAdapter#finalize().
>
The map would also need to handle overloaded setters, so would probably be
class --> property name --> List<Method>, along with a helper method to
iterate over the List and return the Method matching the required parameter
types.

In order to manage the size of the cache, perhaps a LRU cache with a user
configurable size limit would be best? (for the outer map of Map<Class<?>,
String>)  Although I am not sure how much of a problem cache size might be
in any case.

 > Moving the big ugly if blocks into separate StyleDictionary source files
> > would keep the skin sources clean.
>
> You can't really do that, though, since the style fields are private to the
> skin classes themselves.
>
Of course. I forgot about that.  Without wishing to go too far off topic -
shouldn't they really be protected to allow skins to be more easily
extended?

Chris

Re: Some thoughts on BeanAdapter

[Greg Brown <gk...@verizon.net>]

> 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.

Me too. I was actually quite surprised to find that it wasn't. I should have thought of caching the methods at the time, but for some reason it didn't occur to me.  :-P

> 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.

Yeah, I think a static map of Class to property name to method might work well. We could remove class entries from the map in BeanAdapter#finalize().

> 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).

Ah, OK. Still, the maintenance was quite tedious, and the string comparisons required to map property names to fields were not terribly efficient, either, so I think that BeanAdapter is a better choice.

> Moving the big ugly if blocks into separate StyleDictionary source files

> would keep the skin sources clean.

You can't really do that, though, since the style fields are private to the skin classes themselves.

> 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.

I understand the motivation, but it does seem like kind of a pain. I think optimizing BeanAdapter is a much more attractive alternative.

G

Re: Some thoughts on BeanAdapter

[Chris Bartlett <cb...@gmail.com>]

(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...@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

Re: Some thoughts on BeanAdapter

[Greg Brown <gk...@verizon.net>]

> 1. Tooling & Documentation
> There is a lot of useful logic in BeanAdapter and BXMLSerializer which is
> hidden away, but could possibly be exposed. Exposing the bean as BeanAdapter
> 'sees' it with Pivot specifics such as static properties, read only
> Sequences & Maps and ListenerLists would make documenting Components and
> Skins very easy.

Not sure exactly what you have in mind. Could you elaborate?

> BeanAdapter currently exposes a list of properties (via its PropertyIterator
> inner class) but this could be taken further to include the Pivot specifics
> as mentioned above, as well as a means to determine how that property will
> be accessed (direct field access or via an accessor/mutator method), and
> overloaded property setter methods.

As I recall, we eliminated direct field access in Pivot 2.0, so only getter/setters are currently supported.

> Quite understandably, BeanAdapter requires an Object instance. However, some
> refactoring could split BeanAdapter into 2 classes, one which
> extracts/introspects the class, and one which provides Dictionary access to
> instance of the class. Essentially, create a PivotBeanInfo class. This could
> easily then be used for providing the data for a Pivot Doclet &
> ComponentExplorer.

Are you suggesting that BeanAdapter be capable of wrapping a Dictionary? If so, what would the benefit be?

> 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.

> 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.

> 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.

> 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.

> 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.

> One other related point on renderer performance is in regard to the recent
> change to use the JSON class. JSON#get(Object, Sequence<String>) first
> attempts to get the value via BeanAdapter rather than checking whether the
> target class implements Dictionary.

The reason we added that was so we could use JSON.get() to get values like HashMap#getCount(), which otherwise wouldn't be accessible. However, in retrospect it does seem like an edge case, so the performance hit is probably not worth it. It should probably be reverted to checking for Dictionary first.

> 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?

> b) Cache & reuse class data in BeanAdapter

I agree that this would be worthwhile.

> c) Use reflection as a fallback within StyleDictionary

As I mentioned above, I don't think this is a good idea.

> d) Ensure Dictionary implementations are preferred over BeanAdapter
> instances (unless caching sufficiently improves BeanAdapter performance)

Agreed.

G