Add API to shrink memory to Sqlite.jsm

RESOLVED FIXED in mozilla21

Status

()

Toolkit
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 705151 [details] [diff] [review]
Implement shrinkMemory(), v1

I like exposing common PRAGMA statements via named functions.

This will likely eventually be followed up with a periodic or on idle timer. That will be a part 2 or follow-up bug, depending on timing and what Marco wants.

The test is minimal because AFAIK there is no way to obtain current memory usage of the database from JS. And, even if we were to obtain current memory usage, I'm not sure we could reliably guarantee that shrinking memory will actually shrink memory.
Attachment #705151 - Flags: review?(mak77)
(Assignee)

Comment 1

5 years ago
We should also release cached statements. I'm not sure whether we should do this in shrinkMemory() or have a separate function to do that.
Summary: Add API to shrink memory → Add API to shrink memory to Sqlite.jsm
Comment on attachment 705151 [details] [diff] [review]
Implement shrinkMemory(), v1

Review of attachment 705151 [details] [diff] [review]:
-----------------------------------------------------------------

Regardless, it's better to handle statements in a bug apart.
I think that, if we go for release-unused-statements it may be part of shrinkMemory, otherwise if we go for release-all-statements it may be too much.
I'm more prone for the former so that we just drop statements unused by N minutes and then shrinkMemory could "safely" drop them.
On a timer (configurable, so that the consumer can opt-out and do that manually through this API) we may then just invoke shrinkMemory.
Attachment #705151 - Flags: review?(mak77) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 705473 [details] [diff] [review]
shrinkMemory() with optional automatic background shrinking, v2

Now with a background timer to periodically shrink memory on idle! It's currently optional. Although, I think enabling it by default with a large default interval (like 5 or 10 minutes) should be considered.

I agree with previous review that adding statement cleanup should be a follow-up bug.

I also fixed a bug in statement execution. If we threw during _executeStatement() for anonymous statements, we would have leaked the statement.
Assignee: nobody → gps
Attachment #705151 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #705473 - Flags: review?(mak77)
(In reply to Gregory Szorc [:gps] from comment #3)
> Now with a background timer to periodically shrink memory on idle! It's
> currently optional. Although, I think enabling it by default with a large
> default interval (like 5 or 10 minutes) should be considered.

it's 5 or 10 minutes a large interval? I was rather thinking 15-30... the opportunity for a statement to be reused in 5 minutes sounds high.
ah I now got here you meant the idle time.. yes in such a case 5 or 10 minutes is fine... the statements invalidation timer should be higher though.
(Assignee)

Comment 6

5 years ago
(In reply to Marco Bonardo [:mak] from comment #5)
> ah I now got here you meant the idle time.. yes in such a case 5 or 10
> minutes is fine... the statements invalidation timer should be higher though.

Is it such a big deal to invalidate statements earlier? I'm not sure what all goes into creating a statement instance. But if it's little beyond parsing SQL, I don't think optimistic invalidation would be such a big deal.

Also, the problem with default values is getting them correct. In the end, things change depending on the customer (database).
Comment on attachment 705473 [details] [diff] [review]
shrinkMemory() with optional automatic background shrinking, v2

Review of attachment 705473 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +45,5 @@
>   *
> + *   shrinkMemoryOnIdleMS -- (integer) If defined, the connection will attempt
> + *       to minimize its memory usage after this many milliseconds of idle. The
> + *       connection is idle when no statements are executing. There is no
> + *       default value which means no automatic memory minimization will occur.

yeah, I think it's better to make this opt-out rather than opt-in, if this is set to 0 it won't shrink otherwise will default to something like 5-10 minutes.

@@ +204,5 @@
> +  if (this._idleShrinkMS) {
> +    this._idleShrinkTimer = Cc["@mozilla.org/timer;1"]
> +                              .createInstance(Ci.nsITimer);
> +    this._resetIdleShrinkTimer();
> +  }

Ah I see you wanted to manage a local idle rather than use the global system idle.
I'm not sure which solution I prefer, this way there's some risk we hit the user while he's playing an online game or doing some performance-needed operation...
Maybe we should be a bit more on the safe side for the first iteration and rather use a 5 minutes idle returned by Services.idle.addIdleObserver?

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +373,5 @@
> +    },
> +  });
> +
> +  // We reset the idle shrink timer after monkeypatching because otherwise the
> +  // installed timer callback will reference the non-monkeypatched function.

what if we send an observer topic on automatic shrinking? should simplify the test.
(In reply to Gregory Szorc [:gps] from comment #6)
> Is it such a big deal to invalidate statements earlier? I'm not sure what
> all goes into creating a statement instance. But if it's little beyond
> parsing SQL, I don't think optimistic invalidation would be such a big deal.

Well, the most time should be spent in preparing them, that also checks the statement is valid against the database schema, and not sure if it also (probably) runs the optimizer on the statement. Avoiding this preparation is good and that's the primary reason we cache them, rather than parsing.
I'm not sure I'd be willing to pay some additional milliseconds of perf just cause I was reading an article and I took 5 minutes :) not saying we should go for really large values, but I think 15 minutes that a statement is unused would be a good compromise... As usually, hard to tell without data.
(Assignee)

Comment 9

5 years ago
(In reply to Marco Bonardo [:mak] from comment #7)
> @@ +204,5 @@
> > +  if (this._idleShrinkMS) {
> > +    this._idleShrinkTimer = Cc["@mozilla.org/timer;1"]
> > +                              .createInstance(Ci.nsITimer);
> > +    this._resetIdleShrinkTimer();
> > +  }
> 
> Ah I see you wanted to manage a local idle rather than use the global system
> idle.
> I'm not sure which solution I prefer, this way there's some risk we hit the
> user while he's playing an online game or doing some performance-needed
> operation...
> Maybe we should be a bit more on the safe side for the first iteration and
> rather use a 5 minutes idle returned by Services.idle.addIdleObserver?

There are pros and cons to the idle service.

A pro (as you mentioned) is not conflicting with other things the user is doing. A con is that the idle timer may never run (because of a long-running game for example).

Let's take a few databases as an example. For Places, you probably don't want to shrink while the user is engaging with the app because Places is a highly-used database. For extensions, it probably doesn't matter because interactions with this database are seldom and far between. FHR is probably somewhere between.

Usage like Places would benefit from the idler service timer. Usage like extensions would benefit from the idle timer as implemented.

Perhaps we should offer both?

> ::: toolkit/modules/tests/xpcshell/test_sqlite.js
> @@ +373,5 @@
> > +    },
> > +  });
> > +
> > +  // We reset the idle shrink timer after monkeypatching because otherwise the
> > +  // installed timer callback will reference the non-monkeypatched function.
> 
> what if we send an observer topic on automatic shrinking? should simplify
> the test.

Good idea. This will also address the race conditions in the test.
I missed an important point in the patch, the fact you init the timer only on interaction, so basically this means nothing happens until I interact with the db, from the last operation I run there is a timeout that will cleanup whatever I left behind.
I instead thought we were continuously trying to shrink on a timer, even multiple times consecutively.

In such view it's much better looking, and I've always been a fan of adaptive behaviors. So, I changed my mind and think your solution will work fine, I still prefer opt-out though.

That said, I think the name is a bit misleading, "shrinkMemoryOnIdleMS" due to the idle definition in the product... shrinkMemoryOnConnectionIdleMs? I'd like something to make clear the idle is referred to that specific connection.
or just connectionIdleMs and specify on that time we do "housekeeping" that also includes minimize memory usage, and in future we may do more, like vacuuming once a week or dunno.
Comment on attachment 705473 [details] [diff] [review]
shrinkMemory() with optional automatic background shrinking, v2

I will just wait next version, I think we clarified behavior for now.
Attachment #705473 - Flags: review?(mak77)
Whiteboard: [MemShrink]
I propose splitting this in two: the tiny functionality to shrink, and the idle timer. For Bug 832067 we can get away with just the former.
The patch is basically done, I don't think that will save much time.
Comment on attachment 705151 [details] [diff] [review]
Implement shrinkMemory(), v1

Landing this.
Attachment #705151 - Attachment is obsolete: false
Part 1:

https://hg.mozilla.org/services/services-central/rev/3da328ea9e2c
Whiteboard: [MemShrink] → [MemShrink][leave open][part fixed in services]
(Assignee)

Comment 17

5 years ago
Created attachment 706018 [details] [diff] [review]
Shrink on connection idle, v3

Changed name of option to reflect connection idle as opposed to service idle. Added note in docs to reflect this.

Added promises to tests to hopefully avoid race conditions.

Added a new test to ensure the timer is reset as part of executing statements.

We'll tackle a reasonable default value and statement cleanup in follow-up bugs.
Attachment #705473 - Attachment is obsolete: true
Attachment #706018 - Flags: review?(mak77)
Comment on attachment 706018 [details] [diff] [review]
Shrink on connection idle, v3

Review of attachment 706018 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these addressed

::: toolkit/modules/Sqlite.jsm
@@ +207,5 @@
> +  this._idleShrinkMS = options.shrinkMemoryOnConnectionIdleMS;
> +  if (this._idleShrinkMS) {
> +    this._idleShrinkTimer = Cc["@mozilla.org/timer;1"]
> +                              .createInstance(Ci.nsITimer);
> +    this._resetIdleShrinkTimer();

why are we resetting the timer here, there is nothing to clean yet, better to wait the first execution.

@@ +425,5 @@
> +
> +    try {
> +      this._executeStatement(sql, statement, params, onRow).then(
> +        function onResult(result) {
> +          this._resetIdleShrinkTimer();

well, removing the call to reset on open, that is really not needed, everytime you call clear, and then reset.  This way it's not resetting anything, since we cleared the timer, maybe should just be named _startShrinkTimer.

@@ +792,5 @@
> +    this._idleShrinkTimer.initWithCallback({
> +      // shrinkMemory calls execute() which will reset the idle timer on
> +      // statement completion. This reinstalls the timer for us.
> +      notify: this.shrinkMemory.bind(this),
> +    }, this._idleShrinkMS, this._idleShrinkTimer.TYPE_ONE_SHOT);

nsITimerCallback has the [function] annotation, so you don't need an object with notify, just the function.

the problem here is that on timer depletion we call shrink, that executes, and then reset the timer.
so we may have:
bunch of stuff, timer, shrink, timer, shrink, timer, shrink... when clearly there is nothing to shrink.
Attachment #706018 - Flags: review?(mak77) → review+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/services/services-central/rev/2616a8dbe520

With all review comments addressed. I even added a test to ensure we don't call shrink on new connections!

Now on to the follow-ups...
Whiteboard: [MemShrink][leave open][part fixed in services] → [MemShrink][fixed in services]
(Assignee)

Updated

5 years ago
Blocks: 834449
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/mozilla-central/rev/3da328ea9e2c
https://hg.mozilla.org/mozilla-central/rev/2616a8dbe520
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink][fixed in services] → [MemShrink]
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.