504217 - nsZipReaderCache is pointless

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[(dormant account)]

Every time we open a file in a jar file, we stat the filesystem, this sucks up about 100ms on wince for no good reason.

It doesn't sound like there is a good reason to support modifying the jar files while mozilla is running. Infact I doubt this works too well as old nsZipArchive would linger in existing InputStreams.

I propose nuking the cache and making it into something that merely manages existing nsZipArchive objects.

Comment 1

[Benjamin Smedberg]

As long as there's a way to "manually" clear the manager, this sounds fine...

Comment 2

[(dormant account)]

I settled my differences with nszipreadercache in bug 504864

Comment 3

[(dormant account)]

Got bitten by bug 637286 for not taking care of this sorry excuse for a cache sooner.

Comment 4

[(dormant account)]

Attachment: nuke the cache logic

Comment 5

[(dormant account)]

Attachment: bonus nuking

I think we should also remove this cache hitrate crap while at it.

Comment 6

[Michael Wu [:mwu]]

If we had telemetry, we could probably figure out if this causes excessive jar reopening for most users.

Comment 7

[(dormant account)]

(In reply to comment #6)
> If we had telemetry, we could probably figure out if this causes excessive jar
> reopening for most users.

we have the histogram support now. What exactly would you measure?

Comment 8

[Michael Wu [:mwu]]

I think we want to nuke zipreadercache and add a simple, small ziparchivecache which will eliminate our hack to detect omnijars at open time.

Comment 9

[Mike Hommey [:glandium]]

Just thinking out loud here. There is actually a real use case for the zipreadercache: keepingJARs open across several uses of the JAR protocol handler (which is where most JAR accesses go). Without the zipreadercache, we would be opening JARs, and reading their index for each and every jar:some.jar!/path url with a different "path". At least, that's how the JAR protocol handler works at the moment.

Comment 10

[(dormant account)]

(In reply to Mike Hommey [:glandium] from comment #9)
> Just thinking out loud here. There is actually a real use case for the
> zipreadercache: keepingJARs open across several uses of the JAR protocol
> handler (which is where most JAR accesses go). Without the zipreadercache,
> we would be opening JARs, and reading their index for each and every
> jar:some.jar!/path url with a different "path". At least, that's how the JAR
> protocol handler works at the moment.

Yes, but at the moment it does more than this.

Comment 11

[Nicholas Nethercote [inactive]]

nsZipReaderCache is hurting B2G.  See bug 806383.

Comment 12

[Michael Wu [:mwu]]

I don't think this should block basecamp. Fixing this directly involves some risk on all platforms and the discussion in bug 806383 suggest that this probably isn't actually hurting that much.

Comment 13

[Jason Duell]

If and when we remove jar caching we might want to at least still cache the file descriptors for jar files, else we'll need to do IPC to open them on parent and schlep them back to child for every JARChannel (as I'm implementing in bug 815523: jar caching means this happens once per app run instead of per resource).

See also bug 817202--we're currently doing main thread I/O for fopen() in asyncOpen and zipcache amortizes that too.