Andrew, Sorry, there was a glitch in the patch: index is -1 always there. The change is pretty trivial but I attached updated patch in HARMONY-5900.
Thanks, Aleksey. On Wed, Jul 9, 2008 at 3:21 AM, Andrew Cornwall <[EMAIL PROTECTED]> wrote: > Errm, I might take that back. I'm seeing a failure in > ArchiveTest.testWithAnnotations: > > java.lang.NullPointerException > at > org.apache.harmony.unpack200.CpBands.cpNameAndTypeValue(CpBands.java:776) > at > org.apache.harmony.unpack200.MetadataBandGroup.getNextValue(MetadataBandGroup.java:225) > at > org.apache.harmony.unpack200.MetadataBandGroup.getAnnotation(MetadataBandGroup.java:199) > at > org.apache.harmony.unpack200.MetadataBandGroup.getAttribute(MetadataBandGroup.java:167) > at > org.apache.harmony.unpack200.MetadataBandGroup.getAttributes(MetadataBandGroup.java:140) > at > org.apache.harmony.unpack200.ClassBands.parseClassMetadataBands(ClassBands.java:1204) > at > org.apache.harmony.unpack200.ClassBands.parseClassAttrBands(ClassBands.java:518) > at org.apache.harmony.unpack200.ClassBands.unpack(ClassBands.java:152) > at org.apache.harmony.unpack200.Segment.parseSegment(Segment.java:385) > at org.apache.harmony.unpack200.Segment.unpack(Segment.java:405) > at org.apache.harmony.unpack200.Archive.unpack(Archive.java:159) > at > org.apache.harmony.unpack200.tests.ArchiveTest.testWithAnnotations(ArchiveTest.java:147) > > Looks as if something's missing: the compiler is flagging: > cpNameAndType = new CPNameAndType(name, descriptorU, domain, > index.intValue() + descrOffset); > > with: "The variable index can only be null at this location". > > Andrew Jr. > > On Tue, Jul 8, 2008 at 10:03 AM, Andrew Cornwall <[EMAIL PROTECTED]> > wrote: > >> I guess maybe Sian's patch wasn't meant to get search out, but just test if >> the CPUTF8 signatures appeared in the right place? If so, they do - I've >> just confirmed it. I think the signature code may be obsolete. Does this >> mean we can get rid of it? Do we need to do the search (or Aleksey's >> equvialent) if we don't need to distinguish different kinds of CPUTF8? >> >> Aleksey, I can confirm your patch provides a performance boost and does not >> appear to change any behaviour in unpacking. >> >> Andrew Jr. >> >> >> On Tue, Jul 8, 2008 at 9:48 AM, Aleksey Shipilev < >> [EMAIL PROTECTED]> wrote: >> >>> Andrew, >>> >>> There's a hint - search() called for several distinct String[] arrays. >>> Please see my patch, it eliminates search() at all, there you can find >>> the search() consumers. >>> >>> Thanks, >>> Aleksey. >>> >>> On Tue, Jul 8, 2008 at 8:43 PM, Andrew Cornwall >>> <[EMAIL PROTECTED]> wrote: >>> > I applied Sian's patch from HARMONY-5900, but that doesn't appear to >>> change >>> > the runtime characteristics very much. In particular, it appears that >>> > CpBands.search is still being called - did I miss something in the >>> patch? >>> > >>> > Andrew Jr. >>> > >>> > On Tue, Jul 8, 2008 at 9:25 AM, Andrew Cornwall < >>> [EMAIL PROTECTED]> >>> > wrote: >>> > >>> >> Sian, are we still using the DOMAIN_* code for ordering? I thought >>> you'd >>> >> changed the code to use the ordering defined in the pack200 archive? If >>> so, >>> >> we might be able to get rid of all the DOMAIN_ code. >>> >> >>> >> Andrew Jr. >>> >> >>> >> >>> >> >>> >> On Tue, Jul 8, 2008 at 4:05 AM, Aleksey Shipilev < >>> >> [EMAIL PROTECTED]> wrote: >>> >> >>> >>> Ah! IMO, that's ok in terms of performance. >>> >>> Anyway, we can implement Map storage for CpSignatures and have no >>> >>> degradation at all. >>> >>> >>> >>> Let's wait for Andrew. >>> >>> >>> >>> Thanks, >>> >>> Aleksey. >>> >>> >>> >>> On Tue, Jul 8, 2008 at 3:00 PM, Sian January < >>> [EMAIL PROTECTED]> >>> >>> wrote: >>> >>> > I posted a suggested alternative on the JIRA, which doesn't do this >>> >>> search >>> >>> > at all and just uses the same objects for CpSignatures that were >>> >>> transmitted >>> >>> > in CpUtf8 if they exist. The downside of this is that >>> >>> > CpBands.cpSignatureValue(..) >>> >>> > will always be searching a much larger map, so I'm not sure if there >>> are >>> >>> > performance implications from that. I'm also not 100% sure that we >>> >>> don't >>> >>> > need the code I've taken out, but Andrew might be able to answer >>> that. >>> >>> > >>> >>> > On 08/07/2008, Aleksey Shipilev <[EMAIL PROTECTED]> wrote: >>> >>> >> >>> >>> >> This method is the hell for performance. >>> >>> >> It is not only accounts for 15% of CPU time, but instrumentation >>> also >>> >>> >> shows: >>> >>> >> - average size of array is 4700 elements >>> >>> >> - 99% times the search traverses entire array and finds nothing >>> >>> >> >>> >>> >> We should consider move to *Map here :) >>> >>> >> >>> >>> >> Thanks, >>> >>> >> Aleksey. >>> >>> >> >>> >>> >> On Tue, Jul 8, 2008 at 1:30 PM, Sian January < >>> >>> [EMAIL PROTECTED]> >>> >>> >> wrote: >>> >>> >> > The purpose of this code is to get the class constant pool >>> ordering >>> >>> >> right, >>> >>> >> > so that if a signature string has already been transmitted in the >>> >>> CpUtf8 >>> >>> >> > band it has the correct global index. However it might be >>> possible >>> >>> to do >>> >>> >> > the search a different way if this method is taking a long time. >>> E.g >>> >>> if >>> >>> >> I >>> >>> >> > get rid of the code that differentiates between signatures and >>> other >>> >>> >> Ascii >>> >>> >> > values (i.e. replace all occurrences of >>> >>> >> > ClassConstantPool.DOMAIN_SIGNATUREASCIIZ >>> >>> >> > with ClassConstantPool.DOMAIN_NORMALASCIIZ) then my tests seem to >>> >>> pass, >>> >>> >> > although Andrew you wrote that code so it might be that I'm not >>> >>> testing >>> >>> >> all >>> >>> >> > the cases where this is needed. Also there might be worse >>> >>> performance >>> >>> >> > implications from making that change, but I can attach a patch to >>> the >>> >>> >> JIRA >>> >>> >> > if you would like to test it. >>> >>> >> > >>> >>> >> > >>> >>> >> > >>> >>> >> > On 08/07/2008, Aleksey Shipilev (JIRA) <[EMAIL PROTECTED]> wrote: >>> >>> >> >> >>> >>> >> >> >>> >>> >> >> [ >>> >>> >> >> >>> >>> >> >>> >>> >>> https://issues.apache.org/jira/browse/HARMONY-5900?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12611488#action_12611488 >>> >>> >> ] >>> >>> >> >> >>> >>> >> >> Aleksey Shipilev commented on HARMONY-5900: >>> >>> >> >> ------------------------------------------- >>> >>> >> >> >>> >>> >> >> From my measurements for 20Mb .jar.pack.gz file on Sun JDK >>> 1.6.0_05 >>> >>> >> >> -server, CPBands.search accounts for 15% of CPU time. >>> >>> >> >> Let's move the discussion to dev. >>> >>> >> >> >>> >>> >> >> > [classlib][pack200] >>> >>> CpBands.parseCpSignature(Ljava/io/InputStream;) is >>> >>> >> >> hot >>> >>> >> >> > >>> >>> >> >> >>> >>> >> >>> >>> >>> -------------------------------------------------------------------------- >>> >>> >> >> > >>> >>> >> >> > Key: HARMONY-5900 >>> >>> >> >> > URL: >>> >>> >> https://issues.apache.org/jira/browse/HARMONY-5900 >>> >>> >> >> > Project: Harmony >>> >>> >> >> > Issue Type: Wish >>> >>> >> >> > Components: Classlib >>> >>> >> >> > Affects Versions: 5.0M6 >>> >>> >> >> > Environment: All Pack200 HEAD >>> >>> >> >> > Reporter: Andrew Cornwall >>> >>> >> >> > >>> >>> >> >> > The method >>> >>> >> >> >>> >>> >> >>> >>> >>> org/apache/harmony/unpack200/CpBands.parseCpSignature(Ljava/io/InputStream;) >>> >>> >> >> appears to be very hot. I tried initially to optimize it by >>> caching >>> >>> some >>> >>> >> of >>> >>> >> >> its arrays: >>> >>> >> >> > static void clearArrayCache() { >>> >>> >> >> > arrayCache = new SegmentConstantPoolArrayCache(); >>> >>> >> >> > } >>> >>> >> >> > >>> >>> >> >> > private static SegmentConstantPoolArrayCache arrayCache = >>> new >>> >>> >> >> SegmentConstantPoolArrayCache(); >>> >>> >> >> > >>> >>> >> >> > private int search(String[] array, String string) { >>> >>> >> >> > if(array.length > 30) { >>> >>> >> >> > List indexes = >>> arrayCache.indexesForArrayKey(array, >>> >>> >> >> string); >>> >>> >> >> > if (indexes.size() == 0) { >>> >>> >> >> > return -1; >>> >>> >> >> > } >>> >>> >> >> > return ((Integer)indexes.get(0)).intValue(); >>> >>> >> >> > } else { >>> >>> >> >> > for (int i = 0; i < array.length; i++) { >>> >>> >> >> > if(array[i].equals(string)) { >>> >>> >> >> > return i; >>> >>> >> >> > } >>> >>> >> >> > } >>> >>> >> >> > return -1; >>> >>> >> >> > } >>> >>> >> >> > } >>> >>> >> >> > ... but that didn't appear to increase performance. (Maybe all >>> the >>> >>> >> >> searches are done once?) >>> >>> >> >> > Any ideas how to tune parseCpSignature to get it faster? >>> >>> >> >> >>> >>> >> >> -- >>> >>> >> >> This message is automatically generated by JIRA. >>> >>> >> >> - >>> >>> >> >> You can reply to this email to add a comment to the issue >>> online. >>> >>> >> >> >>> >>> >> >> >>> >>> >> > >>> >>> >> > >>> >>> >> > -- >>> >>> >> > Unless stated otherwise above: >>> >>> >> > IBM United Kingdom Limited - Registered in England and Wales with >>> >>> number >>> >>> >> > 741598. >>> >>> >> > Registered office: PO Box 41, North Harbour, Portsmouth, >>> Hampshire >>> >>> PO6 >>> >>> >> 3AU >>> >>> >> > >>> >>> >> >>> >>> > >>> >>> > >>> >>> > >>> >>> > -- >>> >>> > Unless stated otherwise above: >>> >>> > IBM United Kingdom Limited - Registered in England and Wales with >>> number >>> >>> > 741598. >>> >>> > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire >>> PO6 >>> >>> 3AU >>> >>> > >>> >>> >>> >> >>> >> >>> > >>> >> >> >
