1302422 - Investigate whether cache2 disk cache storage can be used for IMAP

Status: NEW

(MailNews Core :: Networking: IMAP, defect)

Description

[Jorg K (CEST = GMT+2)]

Investigate whether cache2 disk cache storage can be used for IMAP.

In bug 1302228 someone complained that messages are repeatedly loaded from the server if the IMAP folder has no local storage.

Perhaps a disk cache could help us.

This depends on bug 1300402 where we need to investigate whether we can change the current state where messages with non-inline parts are *always* retrieved from the server since their cache entries are marked "content modified" (meaning partial or incomplete).

We also need to see whether the cache keys will be permanent enough and will be carried forward to a new session since they contain the nsIImapMailFolderSink::uidValidity.

Comment 1

[Kent James (:rkent)]

I've never really figured out if our goal is to be the jack of all trades, allowing every possible combination of user use case, or if we should just optimize to the most common case. This issue from bug 1302228 seems to me to be a question of whether we take the non-default "do not use offline folders" and optimize for minimal disk space (as currently), or minimal internet usage (as proposed in bug 1302228), or allow either.

My tendency would be to say "allow either" as a hidden option, then provide an addon that enables the alternate option to avoid cluttering the standard UI. So for example we could allow various hidden options that optimize for minimum internet usage, then have an addon that changes the default and perhaps adds some extra monitoring of internet usage. Not everyone agrees that this is a good design choice, but I have done it repeatedly over the years (JunQuilla displays hidden junk information, TaQuilla utilizes hidden bayes traits).

But I am not willing to do that work.

Comment 2

[rsx11m]

Err, we should be using it already for IMAP per bug 439731 since 3.0; isn't that issue described here similar to bug 629738 then, with the cache not reusing the previously cached MIME part?

Comment 3

[Jorg K (CEST = GMT+2)]

We just switched from cache(1) to cache2 in bug 1021843. That was a quick switch-over of interfaces. Cache2 supports memory and disk caching, and so far we're only using memory cache which persists until you shutdown the session.

I thought that going to disk storage instead would be a one line change:
-    rv = cacheStorageService->MemoryCacheStorage(lci, getter_AddRefs(mCacheStorage));
+    rv = cacheStorageService->DiskCacheStorage(lci, false, getter_AddRefs(mCacheStorage));
but trying it, it turns out more difficult. Brief summary:
NS_BASE_STREAM_WOULD_BLOCK returned here:
https://dxr.mozilla.org/comm-central/rev/c482d8a2cd6178a9e5990c07b6c7c09dc63d919d/mailnews/imap/src/nsImapProtocol.cpp#9193
    rv = in->Read(firstBlock, sizeof(firstBlock), &readCount);
    NS_ENSURE_SUCCESS(rv, rv);

Anyway, as comment #0 explains, even with a disk cache, there are problems.

Bug 629738 is different. There I'm fixing the problem that parts are not cached *at all*, so they will be downloaded as many times as they are accessed in the one session.

Anyway, let's ask Honza:
What's involved in going from memory cache to disk cache?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #3)
> We just switched from cache(1) to cache2 in bug 1021843. That was a quick
> switch-over of interfaces. Cache2 supports memory and disk caching, and so
> far we're only using memory cache which persists until you shutdown the
> session.
> 
> I thought that going to disk storage instead would be a one line change:
> -    rv = cacheStorageService->MemoryCacheStorage(lci,
> getter_AddRefs(mCacheStorage));
> +    rv = cacheStorageService->DiskCacheStorage(lci, false,
> getter_AddRefs(mCacheStorage));
> but trying it, it turns out more difficult. Brief summary:
> NS_BASE_STREAM_WOULD_BLOCK returned here:
> https://dxr.mozilla.org/comm-central/rev/
> c482d8a2cd6178a9e5990c07b6c7c09dc63d919d/mailnews/imap/src/nsImapProtocol.
> cpp#9193
>     rv = in->Read(firstBlock, sizeof(firstBlock), &readCount);
>     NS_ENSURE_SUCCESS(rv, rv);


This is the biggest functional difference of cache2.  One of the reasons why it even exists.

The input stream on disk-backed cache entries must be treated only and only as a non-blocking async input stream.  You need nsIInputStreamPump (use nsInputStreamPump::Create) to read the stream.  I.e. read it asynchronously.

> 
> Anyway, as comment #0 explains, even with a disk cache, there are problems.

Sorry, I don't follow.

> 
> Bug 629738 is different. There I'm fixing the problem that parts are not
> cached *at all*, so they will be downloaded as many times as they are
> accessed in the one session.

Not sure what connection is to this bug.

> 
> Anyway, let's ask Honza:
> What's involved in going from memory cache to disk cache?

Answered above.

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> The input stream on disk-backed cache entries must be treated only and only
> as a non-blocking async input stream.  You need nsIInputStreamPump (use
> nsInputStreamPump::Create) to read the stream.  I.e. read it asynchronously.
That's all we need to know, thanks.

> > Anyway, as comment #0 explains, even with a disk cache, there are problems.
> Sorry, I don't follow.
You don't have to, they are problems in our application logic (cache entries of partly retrieved messages are not used/read, so even when using a disk cache we'd have to fix that application problem first).

Thanks again for the answer, the patience and the support!

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #5)
> Thanks again for the answer, the patience and the support!

Always happy to make TB better ;)

Comment 7

[ISHIKAWA, Chiaki]

I have a patch that may solve the use of cache2.
(In reply to Honza Bambas (:mayhemer) from comment #4) 
> (In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #3)
> > We just switched from cache(1) to cache2 in bug 1021843. That was a quick
> > switch-over of interfaces. Cache2 supports memory and disk caching, and so
> > far we're only using memory cache which persists until you shutdown the
> > session.
> > 
> > I thought that going to disk storage instead would be a one line change:
> > -    rv = cacheStorageService->MemoryCacheStorage(lci,
> > getter_AddRefs(mCacheStorage));
> > +    rv = cacheStorageService->DiskCacheStorage(lci, false,
> > getter_AddRefs(mCacheStorage));
> > but trying it, it turns out more difficult. Brief summary:
> > NS_BASE_STREAM_WOULD_BLOCK returned here:
> > https://dxr.mozilla.org/comm-central/rev/
> > c482d8a2cd6178a9e5990c07b6c7c09dc63d919d/mailnews/imap/src/nsImapProtocol.
> > cpp#9193
> >     rv = in->Read(firstBlock, sizeof(firstBlock), &readCount);
> >     NS_ENSURE_SUCCESS(rv, rv);
> 
> 
> This is the biggest functional difference of cache2.  One of the reasons why
> it even exists.
> 
> The input stream on disk-backed cache entries must be treated only and only
> as a non-blocking async input stream.  You need nsIInputStreamPump (use
> nsInputStreamPump::Create) to read the stream.  I.e. read it asynchronously.
> 
> > 

Without creating nsInputStreamPump::Create() [I am not familiar with nsInputStreamPump],
can we use a function as below to repeatedly read the expected amount of data ?
Basically, this function repeats the Read() until all the data is read.
This function is the basis of the patch for
dealing with partial read from remotely mounted file systems when the server load is very high or
a possible network issue occurs.
(See bug 
Bug 1170606 - C-C T-B fixes to take care of short read (part of [META] Failure to deal with short read)
Bug 1170564 - [META] Failure to deal with short read



/* xxx A point to ponder: this may belong in a M-C stream I/O file. */
/* xxx uint32_t: What about 4gb limit??? */
NS_MSG_BASE
nsresult
FullyReadStream(nsIInputStream *input, char *cp, uint32_t len, uint32_t *really_read)
{
  nsresult rv;
  uint32_t n;
  uint32_t remaining = len;
  char *start = (char *)cp;

  *really_read = 0;

  if (remaining <= 0)
    return NS_ERROR_INVALID_ARG;

  while (remaining > 0)
  {
    n = 0;                      // just in case.

#if _ENABLED_EXCEPTION
    // gcc requires -fexceptions to enable exception handling.
    try {
#endif

      rv = input->Read(start, remaining, &n); // we are inside FullyReadStream
#if _ENABLED_EXCEPTION
    }
    catch (nsresult e) {
#if DEBUG
      fprintf(stderr,"(debug) Aieee... input->Read has thrown an exception!. e=%08x\n", (unsigned int) e);
#endif
      if (NS_FAILED(e))
        rv = e;                       // OK
      else
        rv = NS_ERROR_FAILURE;  // somehow e was NS_OK or something.
    }
#endif
    // https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIInputStream#read%28%29

    // This method, i.e., nsIINputStream Read(), returns the number of
    // bytes copied from the stream (may be less than aCount).
    // It returns 0 to indicate end-of-file.  <=== NOTE THIS.
    // Then how error is caught?!
    // Exceptions thrown
    // <other-error>       <=== NOTE THIS
    // On failure.
    // NS_BASE_STREAM_WOULD_BLOCK
    // Indicates that reading from the input stream would block the
    // calling thread for indeterminate amount of time. This exception
    // may only be thrown if isNonBlocking() returns true.

    /* rv == 0 (NS_OK) and n == 0 means EOF */

    if (NS_FAILED(rv) || n == 0)
    {

      /* XXX may want to repeat read returns if errno == EINTR in the
       * future, but such low-level stuff belongs to M-C tree. 
       */

      // Don't continue. We are at likely at the EOF.
      // Dump the stack to examine what routine is trying to read past EOF.

      // Before tightening up the check by adding |&& *really_read ==
      // 0| in the check for EOF below, 
      // this happened, for example, in calls from 
      // 1. nsMsgFilterList::ReadChar in mailnews/base/search/src/nsMsgFilterList.cpp
      //       --- you have to read ahead to see if there is another filter rule.
      //           It reads characters one by one.
      // 2. nsMsgSendPart::Write()    in mailnews/compose/src/nsMsgSendPart.cpp
      //       --- mime attachment seems to be written to an external file, and then
      //           read again. I think you have to read ahead to see if there is
      //           another multi-part. 
      //           It reads a fixed size block and repeat it. At the end of the available data,
      //           on next read, there is no data and this function returns without reading an octet. 

#if DEBUG
#if defined(linux)
        {
#define FRAMEBUFMAX 2046
          static char *_framebuf[FRAMEBUFMAX];
          fprintf(stderr,"(debug) FullyReadStream: we dump stack and return because we could not read required data at all. Maybe at EOF. rv=0x%08x, n=%d\n", (unsigned int) rv, n);
          int nptrs = backtrace ((void**) _framebuf, FRAMEBUFMAX);
          backtrace_symbols_fd ((void **)_framebuf, nptrs, 2);
        }
#endif
#endif

        // BTW, Mac OSX SDK 10.7 returns n==0 and no error when no
        // data is available.  (*STRANGELY* linux 64-bit did not seem
        // to behave similarly.)
        // We may get stuck by trying to read at EOF over and over.
        // We now simulate an error return so that caller
        // can handle this situtaion uniformly across platforms.
        // It seems that the following if condition signals EOF somehow.
        // Note that we need to return the last partial read (*really_read > 0)
        // as success. That is why the second term is in if-expression.

#if DEBUG
        if (n == 0 && !NS_FAILED(rv)) {
          if (*really_read > 0) {
            fprintf(stderr,"(debug) FullyReadStream(): n == 0, !NS_FAILED(rv) and *really_read =%d > 0\n",
                    *really_read);
          }
        }
#endif

        /* Remember that rv == 0 (NS_OK) and n == 0 means EOF for Read() */
        if (n == 0 && *really_read == 0 && !NS_FAILED(rv))
          rv = NS_OK; // NS_ERROR_FAILURE; //<== probably EOF when no data is read in this case.

        // when octets have been read, rv, most likely NS_OK (at EOF)
        // returns unless an exception was thrown.
        return rv;
    }
    else
    {
      remaining -= n;
      start += n;
    }
    *really_read = len - remaining;
  }
#if DEBUG
  /* assertion is too strong since sometimes we call Read to read
     available octets (size unknown in advance) by specifying a large
     enough input buffer. */
  if (*really_read != len) {
    fprintf(stderr,"(debug) FullyReadStream: "
            "*really_read=%u != len=%u\n",
            *really_read, len);
  }
#endif
  return NS_OK;
}


The slightly modified patches from bug (to take care of the bitrot)
have been checked throughly locally and tryserver and they seem to be OK.

The above function repeats the read until the data is all read.
But as far as TB is concerned, there are not much parallelism here (possibly except for GUI handling), and
so there does not seem to be a performance problem, and
above all, this is necessary for |read| against remotely mounted file system
that may return a short read.

I am fairly confident that patch(es) are safe under linux.
However, on try-comm-central,
I saw a couple of errors under OSX and Windows which 
I have not seen under linux locally or on try-comm-central, and
so will check them once my current concern of using buffering for output in TB
is taken care of. ( Bug 1116055 - Performance issue: Failure to use buffered write (comm-central thunderbird) )

Comment 8

[gene smith]

Honza,
As an experiment in TB, I switched over from purely memory cache to using the disk cache (as Jorg asked you about above). It seems to work fine and I see the email messages and/or mime parts in their own cache file. But there are a few things regarding the disk cache that aren't real clear. So I'm asking if you can point to the documentation or possibly explain some details:

1. browser.cache.disk.max_entry_size = 51200 (default). Is this the true maximum size of a cache file, 51.2 MB?
2. If message won't fit in cache entry file (msg bigger than 51.2MB), will the cache entry exist with a partial message?
3. Is there a limit on the number of cache entry files that can exist?
4. browser.cache.disk.smart_size.enabled = true(default). Not sure how this works. How much free disk will/can be used by cache?
5. browser.cache.disk.capacity = 256000/256MB(default). I found that this is n/a if smart_size.enable is true. Correct?
6. Unlike memory cache, disk cache can survives a restart and doesn't get deleted on shutdown. Right? (Might need to add that option to tb to delete disk cache on shutdown.)

With memory cache, some users have problems due to large messages and have to change the default setting in browser.cache.memory.* to make it work (Re: bug 1589649). It appears the the default setting for disk cache (browser.cache.disk.*) are much better and works as-is with large messages or large attachments on messages.