cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Antonio Gallardo" <agalla...@agssa.net>
Subject Re: svn commit: r178778 - /cocoon/branches/BRANCH_2_1_X/src/blocks/forms/java/org/apache/cocoon/ forms/datatype/DynamicSelectionList.java /cocoon/branches/BRANCH_2_1_X/src/blocks/forms/java/org/apache/cocoon/ forms/formmodel/Field.java /cocoon/branches/BRANCH_2_1_X/status.xml
Date Sat, 28 May 2005 01:20:48 GMT
Hi Joerg:

I am glad to see you active on the list again. As I told you in GT2004, I
like your radar over my work. :-)

[comments below]

On Vie, 27 de Mayo de 2005, 19:02, Joerg Heinicke dijo:
> On 27.05.2005 16:05, antonio@apache.org wrote:
>> Author: antonio
>> Date: Fri May 27 07:05:02 2005
>> New Revision: 178778
>>
>> URL: http://svn.apache.org/viewcvs?rev=178778&view=rev
>> Log:
>> Cforms block: Improved dynamic selection list performance inside
>> repeaters.
>
> Sorry, but I absolutely don't like it for the following reasons:

It is OK. :-)

I clearly understand your concerns and I agree with them. As I told
before, I am a newbie inside cform code. Sorry.

My idea was to rewrite this as clean as posible over the weekend. But I
was in a hurry and wanted to know if the idea will work at all. This is
also why I don't committed to the trunk. To allow other people test if
this works.

I only got one reply, I didn't knew if the community was in favor of the
change (I assumed a lazy approval). This code was done mostly as a proof
of concept. To see if this really will improve the performance.

After all, I think there is a good news: The proof was successful. While
Using 3 combos inside a repeater I can see the code runs 2 to 3 times
faster than before. When the repeater grows, there is almost no
performance penality now.

Now, I am also thinking to improve now other pieces, for example, there is
another big time penality inside selection-lists. Again when we are inside
a repeaters. When we change the value of a selection-list using something
like that:

<fd:on-value-changed>
  <fd:javascript>
    myWidget.setSelectionList("cocoon:/mySelectionList.data?id=" + value);
  </fd:javascript>
</fd:on-value-changed>

Here again, the current behavior is to reread the data. And I noted in
some cases is posible the same combo will be there over and over so a
cache improvement will save a lot of time again. But this is another
story. First, I will try to fix the current one.

> 1. You introduced a dependency on the class DynamicSelectionList into
> Field.java where before was only the interface SelectionList.
>
> 2. You introduced a dependency on the classes Repeater and RepeaterRow.
>
> 3. You mixed builder concerns with datatype concerns (copying code from
> DefaultSelectionListBuilder to DynamicSelectionList).
>
> 4. The implementation of DynamicSelectionList got much more complex,
> where I don't see a reason why? What's the difference between building a
> selection list in generateSaxFragment() and in prepareCache()? 90 lines
> of code just for caching?

Agreed. Totally ugly code. I just wanted to touch the less posible files
to see if this works.

> Requirements:
>
> 1. It must be absolute transparent to Field.java (above issues 1 + 2).
>
> 2. No code duplication in sense of copy & paste (above issue 3).
>
> 3. Simple code! No code duplication in sense of having two parts of code
> doing the same (above issue 4).
>
> Solution/suggestion:
>
> What for do we have source validities? Wouldn't help a
> SelecionListHandler storing the SAX events in a SAX buffer and
> resaxifying the events if source validity is still valid and wouldn't it
> fulfill all requirements?

Thanks again for the tip. I will try to do this over this weekend.

Best Regards,

Antonio Gallardo


Mime
View raw message