112312 - JAR uses 32K buffer, affecting startup time

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P2)

Description

[Cathleen]

Comment 1

[Suresh Duddi (gone)]

Attachment: Reduce 32k mallocs from libjar - darin & dp

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 59461 [details] [diff] [review]
Reduce 32k mallocs from libjar - darin & dp

r=dbaron

Comment 3

[Darin Fisher]

Comment on attachment 59461 [details] [diff] [review]
Reduce 32k mallocs from libjar - darin & dp

sr=darin

Comment 4

[Suresh Duddi (gone)]

Fixing this improved startup by 10%  when darin and me measured it.

Comment 5

[Suresh Duddi (gone)]

Fix checked in.

Comment 6

[Suresh Duddi (gone)]

reopened. Installation fails. :-( Need to see why the standalone installer
doesn't like this change

Comment 7

[Suresh Duddi (gone)]

Ccing sean for debugging help on installer. 

Comment 8

[Daniel Veditz [:dveditz]]

The install is complaining about the CRC check which uses
nsZipArchive::TestItem(). Dunno if it's relevant but this routine still
allocates like the one you changed, although now with a smaller buffer.

Maybe Samir would have a clue. There are now reports that the Linux installer is
failing.

Comment 9

[Suresh Duddi (gone)]

So at 6:48am I backed out my changes. There is a bug that still says there are
download errors. http://bugzilla.mozilla.org/show_bug.cgi?id=112631

Comment 10

[Samir Gehani]

I don't see why changing the buffer size would affect CRC checking.  And as long
as the archive is not being tested by the jar: protocol handler the CRC checking
code is not used when opening jars.  Making a similar change to the CRC checking
code would however benefit install but that's a separate issue.

Comment 11

[Daniel Veditz [:dveditz]]

The issue is that in addition to removing (some of) the mallocs dp also shrank
the buffer size. The bufsize change is the only thing that would affect
::TestItem, but it's a crucial difference. At 32K we had few or no files that
took more than one buffer, at 4K we probably have several.  Not all .xpi files
failed though, so there must be some boundary condition that only comes up
occasionally. (Actually it could still be simply overflowing the buffer since we
don't have many files > 4K either.)

dp, here's a potentially easier way to test: Using a broken build click on one
of the broken .xpi files to install it. Before unpacking the archive XPInstall
will also run through nsZipArchive::TestItem so you could try it out without
having to build and debug the installer itself.  I'd recommend the smaller
artext.xpi over browser.xpi because it's probably only failing on a specific
file or two.

A simple way to prove it's in ::TestItem before the tedium of stepping through
the above is to apply your malloc-removing changes but leave the buffer size at
32K -- I'd expect a test like the above to then work.

Comment 12

[Suresh Duddi (gone)]

zlib like odd size buffers. TestItem fails on inflate after it has inflated
completely. Now I understand why the old code was usin 32k-1 and not 32k

I changed the buffer to 4k-1 and things are happy. Thanks for Cathleen for the
dev installer dev environment for debugging this. Patch comming up soon. Will
change TestItem too so installer will benefit from this too.

Comment 13

[Suresh Duddi (gone)]

Attachment: Removes 32k allocations with a 4k-1 buffer size :-)

Comment 14

[Suresh Duddi (gone)]

Attachment: Remove 32k allocs with 4k-1 stack buffers

Comment 15

[Cathleen]

r=cathleen  :-)

Comment 16

[Darin Fisher]

Comment on attachment 59863 [details] [diff] [review]
Remove 32k allocs with 4k-1 stack buffers

sr=darin

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 59863 [details] [diff] [review]
Remove 32k allocs with 4k-1 stack buffers

r=dveditz too

Comment 18

[Suresh Duddi (gone)]

Fix checked in.