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