201226 - nsZipArchive.cpp: footprint: GetModTime is too much??

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Alfred Kayser]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030406
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030406

GetModTime uses dostime and dosdate to first format the two parts
(time and date) into separatly allocated buffers, and puts both into
a third buffer.
Why not directly into one buffer?

Furthermore GetModTime is apparently only used in nsJar.cpp:: RestoreModTime to
decode back into datetime thing for setting the datetime on extracted files.
Why not skip the step of formatting completely, and directly translating the
date/time fields from the zipfile into the number (PRInt64) as required by
SetLastModifiedTime (by some binairy arithmatic). 
This would save the malloc's completely, and also saves on string formatting and
decoding... This saves footprint and increases overall performance.


Reproducible: Always

Steps to Reproduce:

Comment 1

[Alec Flett]

nice. I'll look into this.

Comment 2

[Alfred Kayser]

First and simple step: just removing dosdate/dostime.

/* 
 * Returns formatted date/time e.g. 06/20/1995 21:07
 * NSPR bug parsing dd/mm/yyyy hh:mm
 *        so we use mm/dd/yyyy hh:mm
 */
char *
nsZipItem::GetModTime()
{
    char *nsprstr = (char *) PR_Malloc(17 * sizeof(char));
    if (!nsprstr)
        return nsnull;
       
    PRUIint16 aDate = this->date;
    PRUIint16 aTime = this->time;

    sprintf(nsprstr, "%02d/%02d/%04d %02d:%02d",
        ((aDate >> 5) & 0x0F), (aDate & 0x1F), (aDate >> 9) + 1980);
        ((aTime >> 11) & 0x1F), ((aTime >> 5) & 0x3F));
    
    return nsprstr;
}
 

Comment 3

[Alec Flett]

it would be nice to avoid mallocing the data alltogether, but remember this is
used by standalond .jar as well.. make sure you don't break that.

Comment 4

[Alfred Kayser]

The above change is compatible with the 'old' way:
it uses PR_Malloc, which is in STANDALONE situation translated 
into malloc.

GetModTime is only used in nsJAR.cpp, so we could modify it
that nsJAR passes a (stack) buffer, so we can remove the malloc
all the way. 

The next step would be to skip the text-formatting completely, but 
it would require a new function to translate a ZIP datetime field directly
to a PRTime thing, in prtime.c

Comment 5

[Alec Flett]

prtime.c is not part of mozilla, it is part of nspr - there is no reason to add
zip-time specific code to it. The code you're talking about belongs right here
in this function - we should just make GetModTime return a PRTime, and build the
time up ourselves (and perhaps we'd keep the same mechanism as it does here, and
use nspr to parse it)

something like
PRTime& nsZipItem::GetModTime() {
  char buffer[17];
  PRTime result;
  PR_snprintf(buffer, sizeof(buffer), ...);
  PR_ParseTimeString(buffer, PR_FALSE, &result);
  return result;
}

};

Comment 6

[Alfred Kayser]

Agree: (but add '#ifndef STANDALONE' to GetModTime)

body would then be:
{
  char buffer[17];

  PRUIint16 aDate = this->date;
  PRUIint16 aTime = this->time;

  sprintf(buffer, "%02d/%02d/%04d %02d:%02d",
        ((aDate >> 5) & 0x0F), (aDate & 0x1F), (aDate >> 9) + 1980);
        ((aTime >> 11) & 0x1F), ((aTime >> 5) & 0x3F));
    
  PRTime result;
  PR_ParseTimeString(buffer, PR_FALSE, &result);
  return result;
}

Comment 7

[Alfred Kayser]

Attachment: Patch for GetModTime

A normal cvs diff -uw patch file, from \mozilla\modules\libjar

Comment 8

[Alec Flett]

Comment on attachment 121671 [details] [diff] [review]
Patch for GetModTime

nice! r=alecf

Comment 9

[Alfred Kayser]

Attachment: Improved Patch

This patch also inlines the RestoreModTime function
as it is called once within the same file, and after
the GetModTime change, the function is only a couple of lines.
This results in less code, and less overhead.

Comment 10

[Alec Flett]

over to you alfred...

Comment 11

[Alec Flett]

Comment on attachment 123008 [details] [diff] [review]
Improved Patch

I don't trust sprintf here.. I realize we're using formatting values and all,
but can't we use PR_snprintf() just to be 100% safe?

other than that, this looks great!
sr=alecf if you use PR_snprintf()

Comment 12

[Alfred Kayser]

Attachment: Final patch, using PR_snprintf for safety.

Comment 13

[Alec Flett]

Comment on attachment 123914 [details] [diff] [review]
Final patch, using PR_snprintf for safety.

nice.
sr=alecf

Comment 14

[Mike Kaply [:mkaply]]

fix checked in.

cvs commit: Examining .
Checking in nsJAR.cpp;
/cvsroot/mozilla/modules/libjar/nsJAR.cpp,v  <--  nsJAR.cpp
new revision: 1.106; previous revision: 1.105
done
Checking in nsJAR.h;
/cvsroot/mozilla/modules/libjar/nsJAR.h,v  <--  nsJAR.h
new revision: 1.44; previous revision: 1.43
done
Checking in nsZipArchive.cpp;
/cvsroot/mozilla/modules/libjar/nsZipArchive.cpp,v  <--  nsZipArchive.cpp
new revision: 1.75; previous revision: 1.74
done
Checking in nsZipArchive.h;
/cvsroot/mozilla/modules/libjar/nsZipArchive.h,v  <--  nsZipArchive.h
new revision: 1.36; previous revision: 1.35
done

Comment 15

[Alfred Kayser]

After checkin, the code build succesfully, and footprint saving is about 400
bytes. Marking verified.