1003120 - measure resident memory use of CLD after startup

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[Florian Quèze [:florian]]

My initial testing in bug 971047 seemed to indicate that our emscripten'ed version of CLD is using 16MB of memory at startup, but njn suggested most of it may be only virtual memory:

(Nicholas Nethercote [:njn] from bug 971047 comment #89)

> Actually... the 16 MiB of asm.js elements is clearly the asm.js heap, which
> may only be partially used, in which case the actual physical memory
> consumption may be less than this suggests. (Though virtual memory
> consumption is still a problem on win32.) Can you get 'resident' values with
> this enabled (and in use) and with it off? That would give us an idea of how
> much of that heap memory is committed. Thanks!

We need to verify this.

Comment 1

[Nicholas Nethercote [inactive]]

What's the timeline for shipping this? We need some measurements before doing that.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

The dependencies of bug 996119 will all be resolved before we "ship" the translation feature.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Memory at startup

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Memory after LanguageDetector.jsm loaded

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Memory after LanguageDetector.detectLanguage("") called

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Using about:memory's "Load and diff..." option, the 'resident' statistic at the bottom shows:

Between startup and LanugageDetector.jsm loaded: +5.58 MB
Between LanguageDetector.jsm loaded and LanguageDetector.detectLanguage(""): +4.57 MB
Between startup and LanguageDetector.detectLanguage(""): +10.15 MB

Nicholas, is this what you are looking for?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

By the way, I produced these by building with translation added to the build but with the browser.translation.detectLanguage pref set to false, so that I could explicitly call detectLanguage from the scratchpad.

Comment 8

[Nicholas Nethercote [inactive]]

> Between startup and LanugageDetector.jsm loaded: +5.58 MB
> Between LanguageDetector.jsm loaded and LanguageDetector.detectLanguage(""):
> +4.57 MB
> Between startup and LanguageDetector.detectLanguage(""): +10.15 MB
> 
> Nicholas, is this what you are looking for?

Yes, thanks. Will this 10 MiB hit be incurred for everybody? That would be a lot for a feature that's only likely to be used by a small fraction of our users.

Comment 9

[Florian Quèze [:florian]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Will this 10 MiB hit be incurred for everybody?

I remember discussing with Felipe plans to disable language detection completely if there's no usable translation service. Not sure we have a bug on file for this, bug 1009481 seems related, but not exactly it.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Re-run of memory testing

I re-ran the testing today. Both yesterday's and today's numbers are from a Windows8.1 64-bit machine running a 32-bit build.

Steps for running the test:
> Launch browser
> Confirm that about:config shows that browser.translation.detectLanguage is set to false
> Restart browser
> Open the Scratchpad
> Open about:memory
> Click "Measure and save..."
> Paste `Components.utils.import("resource:///modules/translation/LanguageDetector.jsm").LanguageDetector` in to the Scratchpad and execute it
> Click "Measure and save..."
> Execute `Components.utils.import("resource:///modules/translation/LanguageDetector.jsm").LanguageDetector.detectLanguage("");`
> Click "Measure and save..."

In this new measurement, I clicked on the "Minimize memory usage" button before clicking on "Measure and save...".

Start -> JSM loaded: 9.36 MB ── resident
JSM loaded -> worker running: 3.41 MB ── resident
Start -> worker running: 12.77 MB ── resident

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

I think we can call this done now.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> > Between startup and LanugageDetector.jsm loaded: +5.58 MB
> > Between LanguageDetector.jsm loaded and LanguageDetector.detectLanguage(""):
> > +4.57 MB
> > Between startup and LanguageDetector.detectLanguage(""): +10.15 MB
> > 
> > Nicholas, is this what you are looking for?
> 
> Yes, thanks. Will this 10 MiB hit be incurred for everybody? That would be a
> lot for a feature that's only likely to be used by a small fraction of our
> users.

We're not gonna enable this for everybody in the near future, but we do want to enable it for a fraction of the beta users next month, for a limited experiment.

But the feature in general is not likely to be used only by a small number of users. The majority of our users are in non-english locales and it is usually a top request from them.

Comment 13

[Nicholas Nethercote [inactive]]

> But the feature in general is not likely to be used only by a small number
> of users. The majority of our users are in non-english locales and it is
> usually a top request from them.

In the spirit of not making people pay for features they don't use, it would be really nice if this auto-loaded, or failing that, if it's possible to disable it, or *something*.

Comment 14

[(no longer active)]

I thought that after the extensive discussion in bug 971047, the plan here was to measure the resident memory use of CLD after startup with the JS versus native implementation.  Did something changed in the planning here?

Comment 15

[:Felipe Gomes (needinfo for replies!)]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> > But the feature in general is not likely to be used only by a small number
> > of users. The majority of our users are in non-english locales and it is
> > usually a top request from them.
> 
> In the spirit of not making people pay for features they don't use, it would
> be really nice if this auto-loaded, or failing that, if it's possible to
> disable it, or *something*.

Yes definitely, there will actually be a visible UI option in the Preferences dialog to disable this feature.

We'll be playing with ways to further reduce the memory usage of this library too. It appears there might be a low hanging fruit from setting a smaller HEAP_SIZE during emscripten compilation (yet to be confirmed).

What do you think is a reasonable memory usage on desktop for such a feature? 5MB, 10MB?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #14)
> I thought that after the extensive discussion in bug 971047, the plan here
> was to measure the resident memory use of CLD after startup with the JS
> versus native implementation.  Did something changed in the planning here?

I was unaware of this discussion. Do we have a simple native implementation that we can test with, or is this going to require extra work to get it talking with the JS front-end?

Comment 17

[Florian Quèze [:florian]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #14)
> I thought that after the extensive discussion in bug 971047, the plan here
> was to measure the resident memory use of CLD after startup with the JS
> versus native implementation.  Did something changed in the planning here?

As I said in bug 996119 comment 3, the plan is to measure the performance and memory usage of the asm.js version that we currently have. Once we have all these numbers, we will decide if we are satisfied with them, or want to investigate using a binary version instead.

Comment 18

[Florian Quèze [:florian]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Do we have a simple native implementation
> that we can test with, or is this going to require extra work to get it
> talking with the JS front-end?

In bug 1003125 I'm working on a binary version, but at this point I'm only compiling it to measure the size of the compiled binary. If we want to actually use it, we will need to access it through jsctypes.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> In bug 1003125 I'm working on a binary version, but at this point I'm only
> compiling it to measure the size of the compiled binary. If we want to
> actually use it, we will need to access it through jsctypes.

I think we need a bug on file to make the native code version work well enough for realistic testing, and then compare the memory use characteristics in realistic scenarios. I don't think that bug needs to block the trial (and perhaps not a more widespread release of the feature). Can you file that bug and add it to the backlog?

Comment 20

[Florian Quèze [:florian]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #19)

> Can you file that bug and add it to the backlog?

Filed bug 1014562.