833609 - Add API to shrink memory to Sqlite.jsm

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Gregory Szorc [:gps]]

Attachment: 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.

Comment 1

[Gregory Szorc [:gps]]

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.

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Gregory Szorc [:gps]]

Attachment: 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.

Comment 4

[Marco Bonardo [:mak]]

(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.

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Gregory Szorc [:gps]]

(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 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Marco Bonardo [:mak]]

(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.

Comment 9

[Gregory Szorc [:gps]]

(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.

Comment 10

[Marco Bonardo [:mak]]

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.

Comment 11

[Marco Bonardo [:mak]]

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 12

[Marco Bonardo [:mak]]

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.

Comment 13

[Richard Newman [:rnewman]]

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.

Comment 14

[Marco Bonardo [:mak]]

The patch is basically done, I don't think that will save much time.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 705151 [details] [diff] [review]
Implement shrinkMemory(), v1

Landing this.

Comment 16

[Richard Newman [:rnewman]]

Part 1:

https://hg.mozilla.org/services/services-central/rev/3da328ea9e2c

Comment 17

[Gregory Szorc [:gps]]

Attachment: 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.

Comment 18

[Marco Bonardo [:mak]]

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.

Comment 19

[Gregory Szorc [:gps]]

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...

Comment 20

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/3da328ea9e2c
https://hg.mozilla.org/mozilla-central/rev/2616a8dbe520