340603 - getting table updates consumes large amounts of memory

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

Getting local table updates can consume large amounts of memory because we update as a single batch once the update file downloads (for a file of 8 mb, it uses about 16mb of memory).  Instead, we should do a streaming update that incrementally updates the sqlite db.

Comment 1

[Tony Chang (Google)]

Attachment: v1: interface for streaming updates

This just adds the interface.  I'll update the JS code to use this in a separate patch.

Comment 2

[Darin Fisher]

Comment on attachment 224646 [details] [diff] [review]
v1: interface for streaming updates

bump the UUID for nsIUrlClassifierDBService

> nsUrlClassifierDBServiceWorker::Update(const nsACString& chunk) {

keep opening bracket style consistent.

    +mPendingStreamUpdate("")

no need to initialize this in your ctor.

+        mTableUpdateLines.AppendElement(nsCString(line));

it may be more efficient to just pass |line| to AppendElement since the compiler should be smart enough to use the right nsCString constructor automatically.  if it does that then you avoid a call to the copy constructor.  another choice is to use the version of AppendElement that takes no arguments, and then call Assign on the new element.

+  mPendingStreamUpdate = "";

.Truncate() would be better.


r+a=darin w/ those changes

Comment 3

[Tony Chang (Google)]

Attachment: v2: interface for streaming updates

Adding all the suggestions from Darin.

Comment 4

[Tony Chang (Google)]

Attachment: v3: interface for streaming updates

Let's try this again.  3 files were missing from the last diff ns[I]?UrlClassifierStreamUpdater.{idl,cpp,h}.

Comment 5

[Darin Fisher]

Comment on attachment 224804 [details] [diff] [review]
v3: interface for streaming updates

>+++ toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp	9a48392cf8bfd057f16740bd00d5f9ccf678963d

>+TableUpdateListener::~TableUpdateListener()
>+{
>+}

nit: Maybe just implement this inline in the class decl so it can
be compiled away.


>+  // Copy the data into a nsCString
>+  char *buf = new char[aLength + 1];
>+  PRUint32 readLength;
>+
>+  rv = aIStream->Read(buf, aLength, &readLength);
>+  if (NS_FAILED(rv)) {
>+    NS_ASSERTION((NS_BASE_STREAM_WOULD_BLOCK != rv),
>+                 "The stream should never block."); 
>+    delete buf;
>+    return rv;
>+  }
>+  
>+  buf[aLength] = '\0';
>+  nsCString chunk(buf);
>+  delete buf;
>+
>+  LOG(("Chunk: %s\n\n", chunk.get()));
>+
>+  rv = mDBService->Update(chunk);

OK, a couple comments:

1) you should use "delete[] buf" instead of "delete buf" since
the array was allocated using new[].

2) you can simplify this code quite a bit by doing the following:

  nsCString chunk;
  rv = NS_ConsumeStream(aIStream, aLength, chunk);

Just #include "nsStreamUtils.h" to get this helper function.


>+  nsUrlClassifierStreamUpdater* updater =
>+        NS_REINTERPRET_CAST(nsUrlClassifierStreamUpdater*, context);

NS_STATIC_CAST should work since nsUrlClassifierStreamUpdater inherits
from nsISupports.


>+nsUrlClassifierStreamUpdater::~nsUrlClassifierStreamUpdater()
>+{
>+}

ditto


>+NS_IMETHODIMP
>+nsUrlClassifierStreamUpdater::SetUpdateUrl(const nsACString & aUpdateUrl)
>+{
>+  nsresult rv = NS_NewURI(getter_AddRefs(mUpdateUrl), aUpdateUrl);
>+  if (NS_FAILED(rv)) {
>+    mUpdateUrl = nsnull;
>+    return rv;

It may be a good idea to have this setter keep the state of the object
unchanged when it fails.  That means only assigning to mUpdateUrl if
NS_NewURI created a nsIURI successfully.

Also, bear in mind that NS_NewURI also takes a charset parameter that
tells it how to encode non-ASCII characters (i.e., whether they should
be converted from UTF-8 to Shift-JIS for example before they are %-escaped).
I don't know if it matters for SetUpdateUrl, but you might want to 
consider the consequences of not having a way to convey that charset info
to NS_NewURI.


>+  rv = NS_NewChannel(getter_AddRefs(channel), mUpdateUrl);
>+
>+  if (!mListener) {
>+    mListener = new TableUpdateListener(c);
>+  }
>+
>+  // Make the request
>+  channel->AsyncOpen(mListener.get(), this);
>+  mIsUpdating = PR_TRUE;
>+
>+  return NS_OK;
>+}

This will crash is NS_NewChannel failed for some reason.


r=darin w/ these issues fixed

Comment 6

[Darin Fisher]

Comment on attachment 224804 [details] [diff] [review]
v3: interface for streaming updates

post a revised patch and i'll approve it for FF2

Comment 7

[Tony Chang (Google)]

Attachment: v4: interface for streaming updates

Incorporated all the changes.  Added a comment to the idl file about character encoding for the url.  I guess it's ok to require data providers to use ascii update urls.

Also added a CancelStream method to the DB interface so we can rollback the transaction if the download fails (i.e., status in onStopRequest fails).

Comment 8

[Darin Fisher]

Comment on attachment 225233 [details] [diff] [review]
v4: interface for streaming updates

sorry for the delay on this approval!

Comment 9

[Tony Chang (Google)]

Attachment: v4: bonecho patch

For completeness, adding the bonecho patch.

Comment 10

[Tony Chang (Google)]

Attachment: v4: bonecho patch

Bleh, missed new files.  Correct patch for bonecho.

Comment 11

[Tony Chang (Google)]

"interface for streaming updates" patch is on trunk and branch.  Leaving bug open for second patch that uses this interface from the JS code.

Comment 12

[Tony Chang (Google)]

Attachment: v1: enable streaming updates

This patch actually turns on the use of the streaming interface.

Comment 13

[Tony Chang (Google)]

fixed on branch and trunk