571419 - Tb3 issues read request for each one byte of msgFilterRules.dat

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[WADA:World Anti-bad-Duping Agency]

Attachment: Process Monitor log(CSV) for msgFilterRules.dat and msgFilterRules-1.dat

[Build ID]
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100604 Shredder/3.2a1pre

Tb3 issues read request for each one byte of msgFilterRules.dat.
By design?

Comment 1

[WADA:World Anti-bad-Duping Agency]

Same "read request for each one byte" is observed with Tb 3.0.4, Tb 3.1xpre, Sm 1.1.18, and Sm 2.0.2 too.

Comment 2

[WADA:World Anti-bad-Duping Agency]

Note:
Because a msgFilterRules.dat file looks read only once(upon first message filtering, or first message filter open via UI), and because physical read from HDD is done with a block size and/or with buffering by OS, or at least with a sector size of HDD, this bug won't produce critical performance problem of Tb while normally running.
I'm not sure perfomance impact on big remote msgFilterRules.dat file.

Comment 3

[Wayne Mery (:wsmwk)]

WADA, is this a regression from version 2?

Comment 4

[:aceman]

It is reading 1 char per function call (readchar) in functions like these, from nsIInputStream. Is nsIInputStream->Read buffered?

char nsMsgFilterList::LoadAttrib(nsMsgFilterFileAttribValue &attrib, nsIInputStream *aStream)
{
  char  attribStr[100];
  char  curChar;
  attrib = nsIMsgFilterList::attribNone;

  curChar = SkipWhitespace(aStream);
  int i;
  for (i = 0; i + 1 < (int)(sizeof(attribStr)); )
  {
    if (curChar == (char) -1 || (!(curChar & 0x80) && isspace(curChar)) || curChar == '=')
      break;
    attribStr[i++] = curChar;
    curChar = ReadChar(aStream);
  }
  attribStr[i] = '\0';
  for (int tableIndex = 0; tableIndex < (int)(sizeof(FilterFileAttribTable) / sizeof(FilterFileAttribTable[0])); tableIndex++)
  {
    if (!PL_strcasecmp(attribStr, FilterFileAttribTable[tableIndex].attribName))
    {
      attrib = FilterFileAttribTable[tableIndex].attrib;
      break;
    }
  }  
  return curChar;
}

// If we want to buffer file IO, wrap it in here.
char nsMsgFilterList::ReadChar(nsIInputStream *aStream)
{
  char  newChar;
  PRUint32 bytesRead;
  nsresult rv = aStream->Read(&newChar, 1, &bytesRead);
  if (NS_FAILED(rv) || !bytesRead)
    return -1;
  PRUint32 bytesAvailable;
  rv = aStream->Available(&bytesAvailable);
  if (NS_FAILED(rv))
    return -1;
  else
  {   
    if (m_startWritingToBuffer)
      m_unparsedFilterBuffer.Append(newChar);
    return newChar;
  }
}

Comment 5

[:aceman]

David Bienvenu, any idea how to easily switch this to some buffered reading? Maybe just exchange the nsIInputStream type?

Comment 6

[:aceman]

Seems there is a nsIBufferedInputStream that attaches to nsIInputStream. Maybe that could work.

Comment 7

[:aceman]

Attachment: patch

OK, I have tested this with a 10000 filters (each with one rule), 1.2MB msgFilterRules.dat, on a SSD disk, but the file was cached by the OS already. Opening the filter list took only about 5 seconds so this is not a performance problem. As WADA said, the file is buffered by the OS.

But I attach a patch that converts the nsIInputStream to a buffered one. WADA can you somehow test if there are now less calls in your log?

Comment 8

[David :Bienvenu]

Comment on attachment 618747 [details] [diff] [review]
patch

I haven't tried the patch, but it looks like the right thing to do, thx.

Comment 9

[WADA:World Anti-bad-Duping Agency]

Comment on attachment 618747 [details] [diff] [review]
patch experiment

I think use of NS_NewBufferedInputStream is correct change, but I can say nothing on corectness of the code, although I believe there is no type in the code.

Comment 10

[:aceman]

But you are the bug reporter and you are the only one who can run the process monitor you used so we need your test.

David Bienvenu, can you create a try build for WADA so he can run the patch?

Comment 11

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #10)
> you are the only one who can run the process monitor you used so we need your test.

Anyone can download charge free Process Monitor which is provided by MS, and anyone can legally run Process Monitor on MS Win if he is genuine MS Win user. And I believe similar charge free monitor tool is available on Linux and Mac OS X.

Comment 12

[:aceman]

Exactly, if one has MS Windows.

But you know best what to look for in the monitor;)

Comment 13

[:aceman]

Comment on attachment 618747 [details] [diff] [review]
patch

OK, I have tested the patch with strace on Linux and I see that before the patch there were read() system calls for each byte, after the patch there was one call for each 10K bytes.

Comment 14

[David :Bienvenu]

Comment on attachment 618747 [details] [diff] [review]
patch

thx for the patch.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/31e6a7c45484

Comment 16

[[e-mail]]

Maybe we need a MsgNewBufferedFileInputStream to match our existing MsgNewBufferedFileOutputStream?