MyFaces Core
  1. MyFaces Core
  2. MYFACES-3130

[PERF] Avoid unnecessary AbstractList$Itr instances

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: JSR-314
    • Labels:
      None
    • Environment:
      myfaces core trunk

      Description

      Similar issue: MYFACES-3129

      loop from java 5:

      for (Object object: objects)

      creates new instance of AbstractList$Itr, if objects are instance of ArrayList.

      Similar situation is with explicit .iterator() call.

      In my testcases, it is about ~ 100 000 instances per request/response. Creation itself is cheap, but triggers GC lately.

      I suggest to use old index-style for (i = 0; i < childCount; i++) if possible. Are there any rawbacks of index-based iteration?
      Children is List and as implementation detail we know that it is instance of ArrayList.

      1. MYFACES-3130-example.patch
        7 kB
        Martin Kočí
      2. UIViewRoot-MYFACES-3130.patch
        3 kB
        Martin Kočí
      3. MYFACES-3130-part2.patch
        14 kB
        Martin Kočí

        Activity

        Hide
        Martin Kočí added a comment -

        Patch commited in trunk 2.1. Now I'm going to implement it for rest of code (mostly visitTree method and some internals like ApplicationImpl._handleListenerForAnnotations). I minimize what profiler shows so if your work on some code where ArrayList is 100% sure (like ApplicationImpl._handleListenerForAnnotations) please change it to index-for;

        RandomAccess: (a instanceof RandomAccess) check in production code is unwanted, but we can detect custom implementation of Component.children in initializer code, try to initialize new component, put one child in it and test, if getChildren() returns RandomAccess. I've check RichFaces and Primefaces and they don't use own children list. Trinidad uses ArrayList.

        Our _ComponentChildrenList does not implement RandomAccess, only delegates on one, I'll change it.

        Show
        Martin Kočí added a comment - Patch commited in trunk 2.1. Now I'm going to implement it for rest of code (mostly visitTree method and some internals like ApplicationImpl._handleListenerForAnnotations). I minimize what profiler shows so if your work on some code where ArrayList is 100% sure (like ApplicationImpl._handleListenerForAnnotations) please change it to index-for; RandomAccess: (a instanceof RandomAccess) check in production code is unwanted, but we can detect custom implementation of Component.children in initializer code, try to initialize new component, put one child in it and test, if getChildren() returns RandomAccess. I've check RichFaces and Primefaces and they don't use own children list. Trinidad uses ArrayList. Our _ComponentChildrenList does not implement RandomAccess, only delegates on one, I'll change it.
        Hide
        Jakob Korherr added a comment -

        +1 for that Jan-Kees.

        But I would check for (instanceof RandomAccess) rather than ArrayList (see [1]).

        [1] http://download.oracle.com/javase/1,5.0/docs/api/java/util/RandomAccess.html

        Show
        Jakob Korherr added a comment - +1 for that Jan-Kees. But I would check for (instanceof RandomAccess) rather than ArrayList (see [1] ). [1] http://download.oracle.com/javase/1,5.0/docs/api/java/util/RandomAccess.html
        Hide
        Jan-Kees van Andel added a comment -

        One comment. Since we know some of the collections where the performance gain is substantial, I suggest to put some "instanceof ArrayList" checks in the code and log a warning that an ArrayList is more suitable.

        I know it's not strictly necessary, but at least it warns users for bad performance.

        About the concurrentmodificationexception, why not just do a list.toArray() and then loop over that array? It might even improve performance (not tested)...

        Show
        Jan-Kees van Andel added a comment - One comment. Since we know some of the collections where the performance gain is substantial, I suggest to put some "instanceof ArrayList" checks in the code and log a warning that an ArrayList is more suitable. I know it's not strictly necessary, but at least it warns users for bad performance. About the concurrentmodificationexception, why not just do a list.toArray() and then loop over that array? It might even improve performance (not tested)...
        Hide
        Martin Kočí added a comment -

        If no objections, I'll commit the patch soon!

        Show
        Martin Kočí added a comment - If no objections, I'll commit the patch soon!
        Hide
        Martin Kočí added a comment -

        mail topic: [core] performance: use indices instead of iterator (MYFACES-3130)

        http://www.mail-archive.com/dev@myfaces.apache.org/msg52979.html

        Show
        Martin Kočí added a comment - mail topic: [core] performance: use indices instead of iterator ( MYFACES-3130 ) http://www.mail-archive.com/dev@myfaces.apache.org/msg52979.html
        Hide
        Martin Kočí added a comment -

        Very interesting results: attached patch example reduces:
        1) number of AbstractList$Itr from ~ 100 000 down to ~1500 instances per one render/response
        2) time for one render/response from ~1500ms down to ~900ms

        in my test case( which is really big view already deployed in production at Czech army - it is not a virtual test case).

        Please review those changes, there are questions:

        1) can or cannot List change during interation? If yes, iterator throws ConcurrentModificationException, index based loop does not detect that modification

        2) Index based loop is suitable only for places, where ArrayList instance is used

        Show
        Martin Kočí added a comment - Very interesting results: attached patch example reduces: 1) number of AbstractList$Itr from ~ 100 000 down to ~1500 instances per one render/response 2) time for one render/response from ~1500ms down to ~900ms in my test case( which is really big view already deployed in production at Czech army - it is not a virtual test case). Please review those changes, there are questions: 1) can or cannot List change during interation? If yes, iterator throws ConcurrentModificationException, index based loop does not detect that modification 2) Index based loop is suitable only for places, where ArrayList instance is used

          People

          • Assignee:
            Martin Kočí
            Reporter:
            Martin Kočí
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development