[jira] [Commented] (MYFACES-985) UIData with multihierarchical children inside produces NPE
[ https://issues.apache.org/jira/browse/MYFACES-985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13069636#comment-13069636 ] Leonardo Uribe commented on MYFACES-985: Doing some performance improvements I notice this old issue. Basically, the code proposed is invalid, but just for reference I'll explain what's going on in this case. The general problem is about when it is valid to add components in JSF inside a dataTable. In theory, UIData.setRowIndex do the following: 1. If the component is in row -1 (no row), take a snapshot of the components that implements EditableValueHolder, to restore them later when rows are traversed. 2. If the component is in a row, save the current state of EditableValueHolder components. 3. Move to the selected row. 4. If no state saved found, restore EditableValueHolder components from saved initial state. 5. If state saved found, restore EditableValueHolder components from stored saved state. The algorithm ignores all transient components, because they don't have state (for example, t:inputCalendar uses a transient input component when popup mode is enabled). The problem happens when a user try to create inside a renderer a component that is EditableValueHolder and is not transient. In the moment the snapshot is created, the component does not exists, so the initial state cannot be created and the algorithm will fail. I ignore the reasons why Mojarra code works, but I believe that is not intentional. In JSF 2.1, full row component state was added to UIData, and the same principle applies, but in that case the restriction applies to all components. Note transient component are ignored too. The problem is adding a non transient EditableValueHolder component inside a dataTable row leads to a illegal state, because the initial state is unknown. In JSF 2.0 and upper the most standard way to add a component as structure of other component is use a custom component facelet tag handler. In t:dataTable, this hack is used for detailStampRow and ajaxRowRender features. Also, PostAddToViewEvent can be used to modify the component tree, because it occur before markInitialState, so UIData rowStatePreserved will work well, and PSS algorithm will deal with it gracefully. I'll close this issue as invalid, because everything that can be done for solve this issue was already done, and the reasons why does not work are completely understood. UIData with multihierarchical children inside produces NPE -- Key: MYFACES-985 URL: https://issues.apache.org/jira/browse/MYFACES-985 Project: MyFaces Core Issue Type: Bug Components: General Environment: Tomcat 5.0 JDK 1.4 Reporter: Andrew Kharchenko Assignee: Mathias Broekelmann Fix For: 2.0.0 Attachments: UIData NPE Sample.rar I've found incorrect UIData behaviour under MyFaces which produces NullPointerException on runtime and which works fine under Sun implementation. Here it is: I have a custom component which is extentor from UIInput. This component has UIPanel extentor component as child which is added to children list of UIInput component on rendering. For one's turn, UIPanel extentor has one more UIInput extentor component as child which is added to children list of UIPanel component on rendering. This component works fine standalone, but when it is added to UIData, I have NPE on runtime. Here is the part of listing: java.lang.NullPointerException at javax.faces.component.UIData.restoreDescendantComponentStates(UIData.java:223) at javax.faces.component.UIData.restoreDescendantComponentStates(UIData.java:235) at javax.faces.component.UIData.restoreDescendantComponentStates(UIData.java:235) at javax.faces.component.UIData.restoreDescendantComponentStates(UIData.java:235) at javax.faces.component.UIData.setRowIndex(UIData.java:178) I will also attach sample component's classes, definitions and test page if it will be granted after issue creation. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Created] (MYFACES-3236) UIData performance improvements
UIData performance improvements
---
Key: MYFACES-3236
URL: https://issues.apache.org/jira/browse/MYFACES-3236
Project: MyFaces Core
Issue Type: Improvement
Components: JSR-314
Affects Versions: 2.1.1, 2.0.7
Reporter: Leonardo Uribe
Assignee: Leonardo Uribe
Some days ago there was a discussion on dev list titled:
[core] performance: performance hints
With the following intention (proposed by Martin Koci):
MK is it possible to introduce performance hints in myfaces-core? Hints
MK similar to javax.faces.component.visit.VisitHint but related to
MK performance improvements. Example:
MK For dataTable like:
MK a:dataTable
MK a:column
MK #{aExpression}
MK it's completely unnecessary to save per-row state. Currently there is
no
MK elegant way how to do read-only table (state per-row is always
MK maintained). If user wants (fast) readOnly table, he/she must extend
MK UIData and re-implemenent setRowIndex method. But hint say
MK org.apache.myfaces.core.UIData.saveRowState=false can solve it
MK elegantly - if present (in component.getAttributes()) UIData skips
MK row-state-saving and restoring methods entirely.
MK Lifespan of those hints can be request (faceContext.attributes) or view
MK (component.attributes)
The discussion there was to create or not a hint but a review of the default
algorithm was never done. In theory, UIData.setRowIndex do the following tasks:
1. If the component is in row -1 (no row), take a snapshot of the components
that implements EditableValueHolder, to restore them later when rows are
traversed.
2. If the component is in a row, save the current state of EditableValueHolder
components.
3. Move to the selected row.
4. If no state saved found, restore EditableValueHolder components from saved
initial state.
5. If state saved found, restore EditableValueHolder components from stored
saved state.
After an in-deep analysis, the conclusion was it is not really necessary to
create the hint. Instead, we can create a better algorithm that take advantage
of
the fact that add a non transient EditableValueHolder component inside a
dataTable row on render response time leads to a illegal state.
Since UIData.setRowIndex is a code that is called multiple times, specially for
large datatable sets. Doing some performance tests, it was notice the current
algorithm uses a lot of memory resources. The proposal is do the following:
1. Iterate using index instead iterator for children. This can duplicate the
lines of code
2. Do not create Object[]{null,null} instances, use a private static final
variable.
3. Cache DataListener[].
4. If no EditableValueHolder instances found, skip state saving code, but reset
all ids.
5. Prevent unnecessary calls to getContainerClientId().
6. Provide initial size for created ArrayList instances to reduce memory usage.
7. Iterate over row state using index, instead iterator, and take advantage
instances are ArrayList.
8. Do not instantiate ArrayList, unless it is necessary.
Tests done shows a really big improvement, specially when non
EditableValueHolder component instances are inside the datatable, which is a
very common use case.
Based on this patch we can do other things like:
1. Port this code to UIRepeat.
2. Port this code to Tomahawk.
3. Reuse already created state instances, which can improve performance on
postback requests.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (MYFACES-3236) UIData performance improvements
[
https://issues.apache.org/jira/browse/MYFACES-3236?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Leonardo Uribe resolved MYFACES-3236.
-
Resolution: Fixed
Fix Version/s: 2.1.2
2.0.8
UIData performance improvements
---
Key: MYFACES-3236
URL: https://issues.apache.org/jira/browse/MYFACES-3236
Project: MyFaces Core
Issue Type: Improvement
Components: JSR-314
Affects Versions: 2.0.7, 2.1.1
Reporter: Leonardo Uribe
Assignee: Leonardo Uribe
Fix For: 2.0.8, 2.1.2
Some days ago there was a discussion on dev list titled:
[core] performance: performance hints
With the following intention (proposed by Martin Koci):
MK is it possible to introduce performance hints in myfaces-core? Hints
MK similar to javax.faces.component.visit.VisitHint but related to
MK performance improvements. Example:
MK For dataTable like:
MK a:dataTable
MK a:column
MK #{aExpression}
MK it's completely unnecessary to save per-row state. Currently there
is no
MK elegant way how to do read-only table (state per-row is always
MK maintained). If user wants (fast) readOnly table, he/she must extend
MK UIData and re-implemenent setRowIndex method. But hint say
MK org.apache.myfaces.core.UIData.saveRowState=false can solve it
MK elegantly - if present (in component.getAttributes()) UIData skips
MK row-state-saving and restoring methods entirely.
MK Lifespan of those hints can be request (faceContext.attributes) or
view
MK (component.attributes)
The discussion there was to create or not a hint but a review of the default
algorithm was never done. In theory, UIData.setRowIndex do the following
tasks:
1. If the component is in row -1 (no row), take a snapshot of the
components that implements EditableValueHolder, to restore them later when
rows are traversed.
2. If the component is in a row, save the current state of
EditableValueHolder components.
3. Move to the selected row.
4. If no state saved found, restore EditableValueHolder components from saved
initial state.
5. If state saved found, restore EditableValueHolder components from stored
saved state.
After an in-deep analysis, the conclusion was it is not really necessary to
create the hint. Instead, we can create a better algorithm that take
advantage of
the fact that add a non transient EditableValueHolder component inside a
dataTable row on render response time leads to a illegal state.
Since UIData.setRowIndex is a code that is called multiple times, specially
for large datatable sets. Doing some performance tests, it was notice the
current
algorithm uses a lot of memory resources. The proposal is do the following:
1. Iterate using index instead iterator for children. This can duplicate the
lines of code
2. Do not create Object[]{null,null} instances, use a private static final
variable.
3. Cache DataListener[].
4. If no EditableValueHolder instances found, skip state saving code, but
reset all ids.
5. Prevent unnecessary calls to getContainerClientId().
6. Provide initial size for created ArrayList instances to reduce memory
usage.
7. Iterate over row state using index, instead iterator, and take advantage
instances are ArrayList.
8. Do not instantiate ArrayList, unless it is necessary.
Tests done shows a really big improvement, specially when non
EditableValueHolder component instances are inside the datatable, which is a
very common use case.
Based on this patch we can do other things like:
1. Port this code to UIRepeat.
2. Port this code to Tomahawk.
3. Reuse already created state instances, which can improve performance on
postback requests.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
Re: [core] performance: performance hints
Hi Just for your informatino, I did some really big improvements over the algorithm, making unnecessary add this hint. See: https://issues.apache.org/jira/browse/MYFACES-3236 for details. This was commited on 2.0.x and 2.1.x branch. It could be good if someone can do some performance tests over this one. regards, Leonardo Uribe 2011/4/28 Leonardo Uribe lu4...@gmail.com: Hi Ok, I'll do a resume about the discussion and provide some opinions about that. LU That could be better, and it has sense to put in tomahawk, LU because after all that is the right location for extend LU default jsf components. MK Yes, this is one point of view and I agree with that custom behaviour MK belongs into custom component. I did exactly the same for my component. MK But there are other topics to consider: MK 0) simple presence of performance hints in core does not break the MK compatibility : the *usage* of that hint can break the compatibility - MK so as usual, user must know what that parameter means and what it can MK cause. Yes, the important thing to note is this could not be the default behavior. MK 1) I think JSF implementation can break the specified functionality: MK myfaces did it already with elResolvers sorting for example. But the MK default must be always false for 100% compatibility with JSF MK specification. Yes, note the default algorithm provide the elResolvers in the expected order as the spec says. MK 2) The hint technique is very common : another example from Java EE is MK world JPA Query.setHint Good example. MK 3) Hints are a simple way to realize something this should be in core MK but because of slow specification release cycle, you must wait a year or MK two to get it officially specified in public API. Agreed MK 4) Ease of usage: for example, if you have only one readonly table in MK whole project, creation of custom component for that purpose is an MK overkill: simple f:attribute name=oam.readOnly value=true / is MK much easier. Good idea. I like the proposal of use an attribute like oam.readOnly, because users can see quickly this attribute is MyFaces specific. But note an interesting point, in facelets mode, use f:attribute is not necessary because there exists a MethodRule called org.apache.myfaces.view.facelets.tag.jsf.ComponentRule that is added by default to all component tags and automatically wire all attributes to the attribute map. So this could be valid too: h:dataTable oam.readOnly=true ... Note internally, the value assigned to this property will be a String, not a boolean, because f:attribute or ComponentRule can't do type conversion in this case. MK 5) Internet is full of JSF is slow. Although I know that is completely MK untrue, hinting the core for more performance is a easy way which MK allows users to express all they need without additional dependencies. Agreed. MK So, do you think we really can't put this feature in core? I mean the MK hints feature generally, not readonly UIData - that was only an MK example. Based on the latest information I think it is ok to add it, as long as we use a distinctive name (like oam.readOnly) to indicate the attribute is myfaces specific. Note this new attribute should be included on MyFaces generated facelets tld javadoc. AS Blake's Axiom of Boolean Properties: You will regret making your AS property a boolean. :-) Really it is not a big deal, but good point. Really my opinion is it is possible for UIData to perform better without introduce this hint, and we should aim for that first. It could be good if UIData knows that there are no EditableValueHolder instances on the component tree, just skip state saving automatically (in fact that code will only be executed just once per request). AS Though now that UIData is stuck with a boolean type for this property, AS we don't have a simple way to support #3 (for UIData) that doesn't AS involve adding yet another attribute. AS In any case, I think it is fine to support this optimization one way AS or another (eg. along the lines discussed above). AS Of course, the magical ideal solution would be to come up with some AS clever way to automatically pick the best behavior. Or, failing that, AS it might be good to warn developers when they specify potentially AS conflicting values (such as disabling state saving for tables that AS stamp EditableValueHolders.) I was thinking on put some code in JSF 2.1 for UData.markInitialState, that check if there is EditableValueHolder instances on the row, and if no instance is found, make UIData skip automatically state saving code. Suggestions are welcome regards, Leonardo Uribe 2011/4/27 Jakob Korherr jakob.korh...@gmail.com Hi, I totally agree with Martin here, in all 6 points ;) The standard behavior of MyFaces core must always match with the spec (-- TCK). However, we can provide as
Re: [core] performance: performance hints
Hello Leonardo, can we backport this code to 1.2.x? Regards Bernd On Fri, Jul 22, 2011 at 11:10 PM, Leonardo Uribe lu4...@gmail.com wrote: Hi Just for your informatino, I did some really big improvements over the algorithm, making unnecessary add this hint. See: https://issues.apache.org/jira/browse/MYFACES-3236 for details. This was commited on 2.0.x and 2.1.x branch. It could be good if someone can do some performance tests over this one. regards, Leonardo Uribe 2011/4/28 Leonardo Uribe lu4...@gmail.com: Hi Ok, I'll do a resume about the discussion and provide some opinions about that. LU That could be better, and it has sense to put in tomahawk, LU because after all that is the right location for extend LU default jsf components. MK Yes, this is one point of view and I agree with that custom behaviour MK belongs into custom component. I did exactly the same for my component. MK But there are other topics to consider: MK 0) simple presence of performance hints in core does not break the MK compatibility : the *usage* of that hint can break the compatibility - MK so as usual, user must know what that parameter means and what it can MK cause. Yes, the important thing to note is this could not be the default behavior. MK 1) I think JSF implementation can break the specified functionality: MK myfaces did it already with elResolvers sorting for example. But the MK default must be always false for 100% compatibility with JSF MK specification. Yes, note the default algorithm provide the elResolvers in the expected order as the spec says. MK 2) The hint technique is very common : another example from Java EE is MK world JPA Query.setHint Good example. MK 3) Hints are a simple way to realize something this should be in core MK but because of slow specification release cycle, you must wait a year or MK two to get it officially specified in public API. Agreed MK 4) Ease of usage: for example, if you have only one readonly table in MK whole project, creation of custom component for that purpose is an MK overkill: simple f:attribute name=oam.readOnly value=true / is MK much easier. Good idea. I like the proposal of use an attribute like oam.readOnly, because users can see quickly this attribute is MyFaces specific. But note an interesting point, in facelets mode, use f:attribute is not necessary because there exists a MethodRule called org.apache.myfaces.view.facelets.tag.jsf.ComponentRule that is added by default to all component tags and automatically wire all attributes to the attribute map. So this could be valid too: h:dataTable oam.readOnly=true ... Note internally, the value assigned to this property will be a String, not a boolean, because f:attribute or ComponentRule can't do type conversion in this case. MK 5) Internet is full of JSF is slow. Although I know that is completely MK untrue, hinting the core for more performance is a easy way which MK allows users to express all they need without additional dependencies. Agreed. MK So, do you think we really can't put this feature in core? I mean the MK hints feature generally, not readonly UIData - that was only an MK example. Based on the latest information I think it is ok to add it, as long as we use a distinctive name (like oam.readOnly) to indicate the attribute is myfaces specific. Note this new attribute should be included on MyFaces generated facelets tld javadoc. AS Blake's Axiom of Boolean Properties: You will regret making your AS property a boolean. :-) Really it is not a big deal, but good point. Really my opinion is it is possible for UIData to perform better without introduce this hint, and we should aim for that first. It could be good if UIData knows that there are no EditableValueHolder instances on the component tree, just skip state saving automatically (in fact that code will only be executed just once per request). AS Though now that UIData is stuck with a boolean type for this property, AS we don't have a simple way to support #3 (for UIData) that doesn't AS involve adding yet another attribute. AS In any case, I think it is fine to support this optimization one way AS or another (eg. along the lines discussed above). AS Of course, the magical ideal solution would be to come up with some AS clever way to automatically pick the best behavior. Or, failing that, AS it might be good to warn developers when they specify potentially AS conflicting values (such as disabling state saving for tables that AS stamp EditableValueHolders.) I was thinking on put some code in JSF 2.1 for UData.markInitialState, that check if there is EditableValueHolder instances on the row, and if no instance is found, make UIData skip automatically state saving code. Suggestions are welcome regards, Leonardo Uribe 2011/4/27 Jakob Korherr jakob.korh...@gmail.com Hi, I totally
Re: [core] performance: performance hints
Hi Bernd Yes, I can backport them to 1.2.x. I was waiting if someone was interested on, but that was fast! regards, Leonardo 2011/7/22 Bernd Bohmann bernd.bohm...@atanion.com: Hello Leonardo, can we backport this code to 1.2.x? Regards Bernd On Fri, Jul 22, 2011 at 11:10 PM, Leonardo Uribe lu4...@gmail.com wrote: Hi Just for your informatino, I did some really big improvements over the algorithm, making unnecessary add this hint. See: https://issues.apache.org/jira/browse/MYFACES-3236 for details. This was commited on 2.0.x and 2.1.x branch. It could be good if someone can do some performance tests over this one. regards, Leonardo Uribe 2011/4/28 Leonardo Uribe lu4...@gmail.com: Hi Ok, I'll do a resume about the discussion and provide some opinions about that. LU That could be better, and it has sense to put in tomahawk, LU because after all that is the right location for extend LU default jsf components. MK Yes, this is one point of view and I agree with that custom behaviour MK belongs into custom component. I did exactly the same for my component. MK But there are other topics to consider: MK 0) simple presence of performance hints in core does not break the MK compatibility : the *usage* of that hint can break the compatibility - MK so as usual, user must know what that parameter means and what it can MK cause. Yes, the important thing to note is this could not be the default behavior. MK 1) I think JSF implementation can break the specified functionality: MK myfaces did it already with elResolvers sorting for example. But the MK default must be always false for 100% compatibility with JSF MK specification. Yes, note the default algorithm provide the elResolvers in the expected order as the spec says. MK 2) The hint technique is very common : another example from Java EE is MK world JPA Query.setHint Good example. MK 3) Hints are a simple way to realize something this should be in core MK but because of slow specification release cycle, you must wait a year or MK two to get it officially specified in public API. Agreed MK 4) Ease of usage: for example, if you have only one readonly table in MK whole project, creation of custom component for that purpose is an MK overkill: simple f:attribute name=oam.readOnly value=true / is MK much easier. Good idea. I like the proposal of use an attribute like oam.readOnly, because users can see quickly this attribute is MyFaces specific. But note an interesting point, in facelets mode, use f:attribute is not necessary because there exists a MethodRule called org.apache.myfaces.view.facelets.tag.jsf.ComponentRule that is added by default to all component tags and automatically wire all attributes to the attribute map. So this could be valid too: h:dataTable oam.readOnly=true ... Note internally, the value assigned to this property will be a String, not a boolean, because f:attribute or ComponentRule can't do type conversion in this case. MK 5) Internet is full of JSF is slow. Although I know that is completely MK untrue, hinting the core for more performance is a easy way which MK allows users to express all they need without additional dependencies. Agreed. MK So, do you think we really can't put this feature in core? I mean the MK hints feature generally, not readonly UIData - that was only an MK example. Based on the latest information I think it is ok to add it, as long as we use a distinctive name (like oam.readOnly) to indicate the attribute is myfaces specific. Note this new attribute should be included on MyFaces generated facelets tld javadoc. AS Blake's Axiom of Boolean Properties: You will regret making your AS property a boolean. :-) Really it is not a big deal, but good point. Really my opinion is it is possible for UIData to perform better without introduce this hint, and we should aim for that first. It could be good if UIData knows that there are no EditableValueHolder instances on the component tree, just skip state saving automatically (in fact that code will only be executed just once per request). AS Though now that UIData is stuck with a boolean type for this property, AS we don't have a simple way to support #3 (for UIData) that doesn't AS involve adding yet another attribute. AS In any case, I think it is fine to support this optimization one way AS or another (eg. along the lines discussed above). AS Of course, the magical ideal solution would be to come up with some AS clever way to automatically pick the best behavior. Or, failing that, AS it might be good to warn developers when they specify potentially AS conflicting values (such as disabling state saving for tables that AS stamp EditableValueHolders.) I was thinking on put some code in JSF 2.1 for UData.markInitialState, that check if there is EditableValueHolder instances on the row, and if no instance is
[jira] [Resolved] (MYFACES-1618) Redundant method call to String.intern
[ https://issues.apache.org/jira/browse/MYFACES-1618?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Leonardo Uribe resolved MYFACES-1618. - Resolution: Fixed Fix Version/s: 2.1.2 2.0.8 Assignee: Leonardo Uribe Redundant method call to String.intern -- Key: MYFACES-1618 URL: https://issues.apache.org/jira/browse/MYFACES-1618 Project: MyFaces Core Issue Type: Improvement Affects Versions: 1.2.0-SNAPSHOT Reporter: Bernhard Huemer Assignee: Leonardo Uribe Fix For: 2.0.8, 2.1.2 Attachments: ImplicitObjects-Names.patch Each implicit object defines its name by using a interned string literal, such as for example the class ApplicationImplicitObject [1] does. The JLS declares that literals are interned anyway (String objects have a constant value. String literals-or, more generally, strings that are the values of constant expressions (§15.28)-are interned so as to share unique instances, using the method String.intern.) [2] so I think the additional call to intern() is redundant. [1]: http://svn.apache.org/viewvc/myfaces/core/branches/jsf12/impl/src/main/java/org/apache/myfaces/el/unified/resolver/implicitobject/ApplicationImplicitObject.java?revision=518967view=markup [2]: http://java.sun.com/docs/books/jls/third_edition/html/lexical.html#3.10.5 -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (MYFACES-2889) [PERF] Remove String.intern() calls in FlashELResolver and ImplicitObjectResolver
[ https://issues.apache.org/jira/browse/MYFACES-2889?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Leonardo Uribe resolved MYFACES-2889. - Resolution: Fixed Fix Version/s: 2.1.2 2.0.8 Assignee: Leonardo Uribe [PERF] Remove String.intern() calls in FlashELResolver and ImplicitObjectResolver - Key: MYFACES-2889 URL: https://issues.apache.org/jira/browse/MYFACES-2889 Project: MyFaces Core Issue Type: Improvement Components: JSR-314 Affects Versions: 2.0.2-SNAPSHOT Environment: JBoss AS 6 M4, Parleys.com JSF2 app Reporter: Jan-Kees van Andel Assignee: Leonardo Uribe Fix For: 2.0.8, 2.1.2 I've been doing some profiling and I see pretty much activity in FlashELResolver.castAndIntern() and ImplicitObjectResolver.castAndIntern(). When I replace the return s.intern() lines by return s, both methods have (of course) much better performance. But I'm pretty sure someone put them there with a reason, like memory footprint. However, I don't see any difference in memory footprint. Any ideas? Do we want to keep the intern() calls? -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira
