Closed Bug 1128561 Opened 5 years ago Closed 3 years ago

Try to PRAGMA shrink_memory when we get a TrimMemory notification

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: mfinkle, Assigned: twointofive, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good next bug][lang=java])

Attachments

(2 files)

In bug 411894, desktop added support for shrinking SQLite memory caches in response to a memory-pressure notification. We should do the same for the Java DBs.

Looks like we can use PRAGMA shrink_memory:
http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageService.cpp#353

I saw some Android code that also tries to reduce the cache_size too:
http://androidxref.com/5.0.0_r2/xref/external/chromium_org/sql/connection.cc#350
Blocks: fennec-mem
OS: Mac OS X → Android
Hardware: x86 → All
Mentor: rnewman, michael.l.comella
Whiteboard: [good next bug][lang=java]
Hi. I'd like to have a crack at this bug. I've a couple of questions about the best approach to take though.

1. Presumably the TrimNotification is found in the MemoryMonitor class? 
2. Where would be the best place to add the call to execute the PRAGMA statement? I mean where does the majority of the DB related code hide? I'm guessing we're not going to want to get the SQLiteDatabase instance(s) and perform the exec here in the MemoryMonitor class... 

Being unfamiliar with what's a pretty hefty codebase I'm just after a few pointers of the best places to look and then hopefully I can take it from there. Cheers.
Flags: needinfo?(michael.l.comella)
Richard should be better able to help here.
Flags: needinfo?(michael.l.comella) → needinfo?(rnewman)
Hi Neil,

There are two ways to do this. One is to assume we're running with DBs, and use method calls; the other is to assume we might not be and use broadcasts.


The former:

You can see in StorageReducer that we grab mDB and expire some history.

We could do a similar thing for memory pressure. The PRAGMA call would live in LocalBrowserDB, forwarded on to the ContentProvider, and we'd expose it via a method like BrowserDB.trimMemory(), called in increaseMemoryPressure.


The latter: 

The alternative is loose coupling: have BrowserProvider (and other database-backed content providers) listen for LocalBroadcasts, and send a suitable LocalBroadcast inside MemoryMonitor.increaseMemoryPressure, where the TODO is.


I tend to favor the latter; it only does the work when the ContentProvider is alive, and it makes extension easy.
Flags: needinfo?(rnewman)
I can verify that the PRAGMA is being called, but I haven't been able to verify that it's having any effect. I load some pages and bookmark some, and then from the Android Monitor tab in studio I request "Memory Usage", which has a DATABASES section such as

 DATABASES
      pgsz     dbsz   Lookaside(b)          cache  Dbname
         4     1476            195      292/99/21  /data/user/0/org.mozilla.fennec_klein/files/mozilla/2rjv5piw.default/browser.db
         4     1476            293      271/99/25  /data/user/0/org.mozilla.fennec_klein/files/mozilla/2rjv5piw.default/browser.db (1)
         4     1476            150       79/99/11  /data/user/0/org.mozilla.fennec_klein/files/mozilla/2rjv5piw.default/browser.db (2)

If I then run 'adb shell am send-trim-memory org.mozilla.fennec_klein RUNNING_CRITICAL' I can verify that the PRAGMA call is being made, but the last cache number (21, 25, 11 above) doesn't change when I rerequest "Memory Usage".  Based on https://github.com/android/platform_frameworks_base/blob/4535e11fb7010f2b104d3f8b3954407b9f330e0f/core/java/android/database/sqlite/SQLiteDebug.java#L142 I'm assuming that last number is cache size.  But maybe "cache size" doesn't mean what I think it does, or something else...  Any suggestions?
Mentor: rnewman → gkruglov
Comment on attachment 8776282 [details]
Bug 1128561 - Do a PRAGMA shrink_memory when we get a TrimMemory notification.

Hey Tom.

I think Grisha would be the most appropriate person to take a look here though he's out for a bit so it'll be a few days before he's able to get back to you.
Attachment #8776282 - Flags: review?(rnewman) → review?(gkruglov)
Assignee: nobody → twointofive
(In reply to Tom Klein from comment #5)
> I can verify that the PRAGMA is being called, but I haven't been able to
> verify that it's having any effect. I load some pages and bookmark some, and
> then from the Android Monitor tab in studio I request "Memory Usage", which
> has a DATABASES section such as
> 
>  DATABASES
>       pgsz     dbsz   Lookaside(b)          cache  Dbname
>          4     1476            195      292/99/21 
> /data/user/0/org.mozilla.fennec_klein/files/mozilla/2rjv5piw.default/browser.
> db
>          4     1476            293      271/99/25 
> /data/user/0/org.mozilla.fennec_klein/files/mozilla/2rjv5piw.default/browser.
> db (1)
>          4     1476            150       79/99/11 
> /data/user/0/org.mozilla.fennec_klein/files/mozilla/2rjv5piw.default/browser.
> db (2)
> 
> If I then run 'adb shell am send-trim-memory org.mozilla.fennec_klein
> RUNNING_CRITICAL' I can verify that the PRAGMA call is being made, but the
> last cache number (21, 25, 11 above) doesn't change when I rerequest "Memory
> Usage".  Based on
> https://github.com/android/platform_frameworks_base/blob/
> 4535e11fb7010f2b104d3f8b3954407b9f330e0f/core/java/android/database/sqlite/
> SQLiteDebug.java#L142 I'm assuming that last number is cache size.  But
> maybe "cache size" doesn't mean what I think it does, or something else... 
> Any suggestions?

Hi Tom,

I'm not actually sure what these numbers represent - I wasn't able to find this data in the Android Device Monitor. How did you access it?

As to why the numbers don't move after pragma is executed, I can imagine a few possibilities:
- (unlikely, but...) you're trying this on an API15 device; IIRC, these run sqlite version 3.7.4 (may vary per device/manufactured/etc), and shrink_memory pragma was introduced in 3.7.10 (https://sqlite.org/changes.html#version_3_7_10) - so it'll be a no-op
- basic browsing activity isn't enough to cause significant memory usage (via cached pages sqlite loads into memory.. what else?). Perhaps try running a sync on a larg-ish profile, and see if you notice a change? Regular browsing is primarily "read"-heavy (a few inserts/updates to track visits, favicon changes, history metadata changes, but mostly various selects to power search and various home panels).
- try removing bunch of history/bookmarks, or generally performing a bunch of writes (again, sync a lot of data) and triggering the shrink_memory pragma after that. Any difference?

Chromium does (or, used to do) something interesting to get their sqlite memory usage down. They significantly drop number of cache pages sqlite is allowed to use (1/2 of regular value, or simply to 1 page on aggressive reduction), and then reset the number back to its original value. See http://androidxref.com/5.0.0_r2/xref/external/chromium_org/sql/connection.cc#350 - perhaps investigate doing this as well and see what effect this has? Although, this feels pretty hacky.

As you're testing this, try triggering low memory notifications _during_ a database-heavy operation such as syncing, and see if this has any interesting side-effects. Device is low on memory (you have a bunch of apps active, sync is running...), we get a low memory notification, we start shedding memory at the expense of read latency (if there's no cache in memory sqlite now needs to go to disk), things start to slow down significantly (but hopefully we lowered the memory pressure) until cache builds up again, etc.
Comment on attachment 8776282 [details]
Bug 1128561 - Do a PRAGMA shrink_memory when we get a TrimMemory notification.

https://reviewboard.mozilla.org/r/68144/#review66454

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:288
(Diff revision 1)
>  
>          // Combined pinned sites, top visited sites, and suggested sites
>          URI_MATCHER.addURI(BrowserContract.AUTHORITY, "topsites", TOPSITES);
>      }
>  
> +    public static final String ACTION_SHRINK_MEMORY = "org.mozilla.gecko.db.intent.action.SHRINK_MEMORY";

nit: move to the top

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:289
(Diff revision 1)
>          // Combined pinned sites, top visited sites, and suggested sites
>          URI_MATCHER.addURI(BrowserContract.AUTHORITY, "topsites", TOPSITES);
>      }
>  
> +    public static final String ACTION_SHRINK_MEMORY = "org.mozilla.gecko.db.intent.action.SHRINK_MEMORY";
> +    private final BroadcastReceiver mShrinkMemoryReceiver = new BroadcastReceiver() {

nit: I don't think we use hungarian notation anywhere in this class, so don't use it here either.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:295
(Diff revision 1)
> +        @Override
> +        public void onReceive(Context context, Intent intent) {
> +            final PerProfileDatabases<BrowserDatabaseHelper> databases = getDatabases();
> +            // Make sure we haven't been shutdown since the broadcast was sent:
> +            if (databases != null) {
> +                ThreadUtils.postToBackgroundThread(new Runnable() {

Instance of this runnable will have an implicit reference to its parent class, which in turn has an implicit reference (as an inner class) to its parent. Any chance you might be leaking stuff here? Can this runnable outlive its parents, somehow?

Consider making instance of the BroadcastReceiver static, and use a WeakReference to BrowserProvider instance.

::: mobile/android/base/java/org/mozilla/gecko/db/PerProfileDatabases.java:88
(Diff revision 1)
>              mStorages.put(profile, helper);
>              return helper;
>          }
>      }
> +
> +    public void shrinkMemory() {

might just synchronize on the method
Attachment #8776282 - Flags: review?(gkruglov) → review+
Thanks for the review and suggestions - I think the WeakReference changes could use another look, could you do another review?  Thanks!
(In reply to :Grisha Kruglov from comment #7)
>
> I'm not actually sure what these numbers represent - I wasn't able to find
> this data in the Android Device Monitor. How did you access it?
> 
It wasn't from Device Monitor, it was from Android Monitor directly in Studio: it's the "Memory Usage" menu item described here: https://developer.android.com/studio/profile/am-sysinfo.html

I still don't see any documentation on what those numbers mean though :(
I did five test scenarios, each consisting of five runs, as follows (copied from the attachment):

*** Each run consists of the following:
      1. Start new instance of fennec;
      2. sign in;
      3. sync ~3500 bookmarks (takes ~1-2 minutes (it varies));
      4. get memory stats;
      5. send pressure: "adb shell am send-trim-memory org.mozilla.fennec_klein RUNNING_CRITICAL";
      6. get memory stats again;
      7. disconnect sync;
      8. close fennec windows and force stop app ***

Each number in the lists of five below is the difference in TOTAL memory (from the stats pages) between
steps 4. and 6. (4. has always been larger than 6.) - I did five runs for each
setup:

  With "PRAGMA shrink_memory": 3114/8103/3521/8161/9028 --> average 6385.4
  Without ShrinkMemory (comment that function out): 2674/7042/3562/6203/8163 --> 5528.8
  [numbers got larger here, don't know why]
  With "PRAGMA cache_size=1": 9659/12490/8469/11723/4842 --> 9436.6
  With "PRAGMA shrink_memory" (again): 4087/11135/8843/13134/10504 --> 9540.6
  Without ShrinkMemory (comment that function out again): 9109/12890/8062/10467/6647 --> 9435

In the first two scenarios it looks like shrink_memory won about 850KB, and in the second group it won about 100KB.  My bookmarks file as json is 1.3M, so maybe the first grouping would be reasonable, if the second had backed it up.  I'm not real confident in drawing any conclusion...  Maybe I just need a larger profile, or maybe someone with more knowledge can get more out of the complete memory reports in the attachment.  The presumptive "cache size" (the third number #/#/# under DATABASES/cache) doesn't change after memory pressure in any of the runs that I looked at; I still don't know what that number actually means.

Triggering the memory pressure event in the middle of syncing doesn't have any noticeable effect, at least that I could tell.  Of course I'm on an emulator, so pretty much everything seems slow :P

Any thoughts on these results?  Or other suggestions?  Thanks!
Comment on attachment 8776282 [details]
Bug 1128561 - Do a PRAGMA shrink_memory when we get a TrimMemory notification.

https://reviewboard.mozilla.org/r/68144/#review66454

> nit: I don't think we use hungarian notation anywhere in this class, so don't use it here either.

There's 'sTables', so I stuck with the hungarian notation.
Okay, looks like "cache size" is actually sqlite's prepared statement cache size:
https://github.com/android/platform_frameworks_base/blob/4535e11fb7010f2b104d3f8b3954407b9f330e0f/core/java/android/database/sqlite/SQLiteConnection.java#L1172

Apparently doing a shrink_memory has no effect on that.  Looking at the memory stats output more though, it does seem to have an effect on the SQL MEMORY_USED stats, which, according to

https://github.com/android/platform_frameworks_base/blob/4535e11fb7010f2b104d3f8b3954407b9f330e0f/core/java/android/database/sqlite/SQLiteDebug.java#L92

is
/** the current amount of memory checked out by sqlite using sqlite3_malloc().
  * documented at http://www.sqlite.org/c3ref/c_status_malloc_size.html
  */

If I record the differences in that MEMORY_USED stat before and after triggering memory pressure, I get:
  With "PRAGMA shrink_memory": 3535/3535/3535/3525/3540 --> 3534
  Without shrinkMemory (comment that function out): 0/860/15/15/0 --> 178
  With "PRAGMA cache_size=1": 15/10/15/865/0 --> 179
  With "PRAGMA shrink_memory" (again): 2705/2575/2555/2550/2555 --> 2588
  Without shrinkMemory (comment that function out again): 865/15/15/15/865 --> 355

So it looks like the shrink_memory is clearing around 3000.  But as far as I can tell that number is in bytes[1]...  Well, at least this seems to be stronger evidence that the "PRAGMA shrink_memory" is doing _something_ :P  I don't know why the "PRAGMA cache_size=1" doesn't seem to be; maybe the change doesn't take effect right away?  (For testing purposes I was just setting cache_size=1 and leaving it there (followed shortly thereafter by force stopping fennec).)

[1] http://sqlite.1065341.n5.nabble.com/what-is-the-unit-of-SQLITE-STATUS-MEMORY-USED-td47012.html
And the docs don't specify, but do seem to indicate bytes: https://www.sqlite.org/c3ref/c_status_malloc_size.html
Comment on attachment 8776282 [details]
Bug 1128561 - Do a PRAGMA shrink_memory when we get a TrimMemory notification.

https://reviewboard.mozilla.org/r/68144/#review69612

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:305
(Diff revision 2)
> +            final PerProfileDatabases<BrowserDatabaseHelper> databases = mDatabasesRef.get();
> +            if (databases != null) {
> +                ThreadUtils.postToBackgroundThread(new Runnable() {
> +                    @Override
> +                    public void run() {
> +                        databases.shrinkMemory();

Something like:
```
final BrowserProvider browserProvider = browserProviderWeakReference.get();
if (browserProvider == null) {
  return;
}
final ... databases = browserProvider.getDatabases();
if (databases == null) {
  return;
}
databases.shrinkMemory();
```

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:312
(Diff revision 2)
> +                });
> +            }
> +        }
> +    }
> +
> +    private static ShrinkMemoryReceiver mShrinkMemoryReceiver;

Doesn't need to be static (inner class must be static, as it now is), but this should be final.

You can initialize it right away if you pass it a reference to `this`, and create a WeakReference to BrowserProvider in `ShrinkMemoryReceiver`'s constructor.

I feel that this would be a tad cleaner.
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b336ce57d884
Do a PRAGMA shrink_memory when we get a TrimMemory notification. r=Grisha
https://hg.mozilla.org/mozilla-central/rev/b336ce57d884
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.