839822 - Perf review for FHR healthreport/

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Just as bug 836719, but different directory.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: First look at healthreport/

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 712549 [details] [diff] [review]
First look at healthreport/

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

Thanks for the feedback! Quick run-through below; gps will have more details.

::: services/healthreport/healthreporter.jsm
@@ +31,5 @@
>  // 2012 and no dates older than that should be encountered.
>  const OLDEST_ALLOWED_YEAR = 2012;
>  
>  const DAYS_IN_PAYLOAD = 180;
> +// Does this mean that we submit data exactly every 180 days?

No, that there are (at most) 180 days of data in each payload.

FHR uses a monolithic document: each client uploads all of their historical data each time, because the server has one record per active client. That monolithic document holds up to 180 days of data.

@@ +93,5 @@
>   *        (string) The preferences branch to use for state storage. The value
>   *        must end with a period (.).
> +// Wait, storing state in preferences? What kind of state is that?
> +// I have no benchmark to back this, but from what I hear, preferences
> +// are considered mighty slow, not to mention main-thread io.

Some state is stored in prefs to avoid opening the database to record it. See Bug 827157 comment 2.

@@ +319,5 @@
>      this._collectorInProgress = false;
>  
>      if (this._shutdownRequested) {
> +// Is that in case of race condition, i.e. shutdown before initialization
> +// is complete?

Yes.

@@ +750,5 @@
>      throw new Task.Result(JSON.stringify(o));
> +// Do you already have any idea how large |o| can be?
> +// FWIW, in Session Restore, we have occurrences of
> +// JSON.stringify that last 1000ms+.
> +// Do you think we need to wait for bug 832664?

I don't believe that waiting for new work is an option, alas.

@@ +751,5 @@
> +// Do you already have any idea how large |o| can be?
> +// FWIW, in Session Restore, we have occurrences of
> +// JSON.stringify that last 1000ms+.
> +// Do you think we need to wait for bug 832664?
> +// Also, I would suggest some Telemetry here.

Agreed.

@@ +832,5 @@
>      let profD = OS.Constants.Path.profileDir;
>  
>      // Work around bug 810543 until OS.File is more resilient.
> +// I resent that comment :)
> +// This has been fixed for FF20, btw.

Great!

::: services/healthreport/profile.jsm
@@ +149,5 @@
>            date = info.lastModificationDate;
>          }
> +// Why not just
> +//  date = info.winBirthDate || info.macBirthDate || info.lastModificationDate
> +// ?

Because:

[12:30:13.438] let d = new Date(0);
[12:30:13.440] undefined
[12:30:16.853] d.getTime();
[12:30:16.855] 0
--
[12:30:22.501] d && true;
[12:30:22.503] true

a zero Date is not falsy. Also it's an opportunity to log and to add a comment explaining what's going on.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 712549 [details] [diff] [review]
First look at healthreport/

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

::: services/healthreport/healthreporter.jsm
@@ +77,5 @@
>   * The handler for "profile-do-change" may result in event loop spinning. This
>   * is because of race conditions between our shutdown code and application
>   * shutdown.
> +// Ouch, that sounds rather scary.
> +// Is this the kind of situation that would be helped by bug 722648?

Yes. We resort to spinning because no proper alternative is available.

@@ +93,5 @@
>   *        (string) The preferences branch to use for state storage. The value
>   *        must end with a period (.).
> +// Wait, storing state in preferences? What kind of state is that?
> +// I have no benchmark to back this, but from what I hear, preferences
> +// are considered mighty slow, not to mention main-thread io.

See bug 834549 comment #3 for Vladan's take on this.

@@ +277,5 @@
>      this._storageInProgress = true;
>      Metrics.Storage(this._dbName).then(this._onStorageCreated.bind(this),
>                                         this._onInitError.bind(this));
> +// I suspect that this whole asynchronous initialization be much easier
> +// to read as a single Task.js task.

Perhaps. Although it would actually be multiple tasks with a few nested try..catch blocks.

@@ +495,5 @@
>    _performDailyMaintenance: function () {
>      this._log.info("Request to perform daily maintenance.");
> +// So daily maintenance removes all data before six months ago?  Do we
> +// know how expensive this operation is? Would it be useful to batch
> +// it e.g. store data per month and remove one month at a time?

It removes all data older than 180 days. This operation just issues a bunch of DELETE SQL queries. All of our SQL queries are performed off the main thread, so jank shouldn't be an issue.

I don't believe storing data per month would be advantageous to storing everything in one table and letting SQL/SQLite sort it out at query time.

@@ +591,5 @@
>        for (let promise of promises) {
> +// I take it this loop is placed here to encourage the providers to initialize
> +// in parallel rather than sequentially, is that it? If so, a little comment
> +// might be useful. If not, you could just put everything in one bigger
> +// Task.spawn and let the promises be handled transparently.

Yes, it is to allow parallel execution. I wish we had a PromiseUtils.waitForAll(promisesArray, timeout) function or something...

@@ +642,5 @@
> +//   Task.spawn(function() {
> +//     /*your actual code*/
> +//   })
> +// This goes double whenever you have some closures-as-private-methods
> +// calling additional closures-as-private-methods.

I did this both to avoid the extra levels of indentation and to allow some internal functions to be called in different contexts. e.g. in the case of ensuring constant-only providers are registered, we want that handled in the public/entry API so multiple internal functions can be chained together (to prevent redundant registering per public API call).

@@ +804,5 @@
>  
>      return Task.spawn(function doUpload() {
>        let payload = yield this.getJSONPayload();
>        yield this._saveLastPayload(payload);
> +// Why do you need to wait until the payload has been written?

We probably don't. It doesn't really harm anything. We may actually remove local saving of the full payload for perf reasons (at least on desktop).

@@ +897,5 @@
>          return Promise.resolve(json);
>        },
>        function onError(error) {
>          return Promise.reject(error);
> +// That function is a noop, isn't it?

It certainly looks like it :)

::: services/healthreport/providers.jsm
@@ +544,5 @@
>      }
>  
>      // Exceptions are caught in the caller.
>      let result = JSON.parse(data.get("addons")[1]);
> +// Do we have an idea of the size of this object?

It's proportional to the number of add-ons installed. On my profile with ~25 add-ons it's ~5k.

@@ +867,5 @@
> +// } catch (ex if ex == StopIteration) {
> +//   // Do nothing
> +// } finally {
> +//   iterator.close();
> +// }

I believe this was also a workaround to OS.File kludges.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Toying with Task.jsm

Gps, here is the patch. Nothing tested, I was just toying with Task.jsm to confirm my intuition that  it could be used to improve readability.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 714039 [details] [diff] [review]
Toying with Task.jsm

I'll keep this patch in mind when we eventually refactor the startup sequence (since the resulting code is much nicer). Since I don't believe it offers any compelling performance advantages (just cosmetics), it could be a while before there is traction.