uckelman at nomic.net
Mon Feb 1 05:22:35 PST 2010
Thus spake Alan Bateman:
> Joel Uckelman wrote:
> > Two more zipfs patches:
> > * Implemented readAttributes().
> > * copyTo() should return target, not this.
> Thanks for taking these bugs. The map key is the attribute name rather
> than view:attribute and so I assume map.put(attr, ..) should be
> map.put(name, ...).
> Another comment is that you've implemented it to use
> getAttribute, and if I read the patch correctly, it means that a bulk
> read turns into a sequence of gets where each reads the zip entry. With
> caching this might not be noticeable but I'm interested to know if
> you've compared it to reading the zip entry once and then populating the
> map with the requested attributes.
readAttribtues does delegate to getAttribute, yes. The problem I see
with reading the ZipEntry once is that the stuff which handles
ZipEntries isn't visible to ZipFilePath---all of the handling code is
burried inside the the three possible Attributes classes
(ZipFileAttributes, JarFileAttributes, ZipFileBasicAttributes).
Since ZipEntries are cached, I'm not worried about overhead of getting
the ZipEntries multiple times; rather, I'm worried about the huge amount
of gargage which will be generated by calling readAttributes: For each
attribute we read, we construct a new View and a new Attributes object.
Since the Views for zipfs are immutable objects, we could store them in
ZipFilePath as volatile members, so they don't need to be recreated
each time they're asked for. We could also cache the Attributes objects,
but in a read-write scenario those could go stale. (This is problem is
easy to solve in my RW implementation, since ZipEntries can go stale,
too, and I'm already handling that.)
What I'm unsure about, and unsure how to even test, is whether caching
this stuff is worth it. The Views, at least, should be among the
cheapest objects to create. Maybe they should just be cached locally,
within readlAttributes, so that each call creates at most one of each?
More information about the nio-dev