Closed Bug 864932 Opened 7 years ago Closed 6 years ago

Rewrite wifi workers in C++

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(blocking-b2g:-, b2g18?)

RESOLVED FIXED
blocking-b2g -
Tracking Status
b2g18 ? ---

People

(Reporter: justin.lebar+bug, Assigned: fabrice)

References

(Depends on 1 open bug)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(3 files, 4 obsolete files)

The wifi JS worker threads use a lot of memory in B2G.  I strongly suspect that if we rewrote them in C++, we'd be able to reduce their memory usage significantly.

Here's a dump of the worker threads' memory usage.  Most-to-all of unused-arenas will go away after bug 829482, but even after that this workers use way too much memory.

> 2.29 MB (06.68%) -- worker(resource://gre/modules/wifi_worker.js, 0x43b3c400)
> ├──1.78 MB (05.19%) -- gc-heap
> │  ├──1.75 MB (05.10%) ── unused-arenas
> │  └──0.03 MB (00.09%) -- (2 tiny)
> │     ├──0.03 MB (00.09%) ── chunk-admin
> │     └──0.00 MB (00.00%) ── unused-chunks
> └──0.51 MB (01.49%) -- (3 tiny)
>    ├──0.21 MB (00.61%) -- compartment(web-worker)
>    │  ├──0.13 MB (00.36%) -- gc-heap
>    │  │  ├──0.07 MB (00.20%) ── unused-gc-things [2]
>    │  │  ├──0.03 MB (00.08%) -- objects
>    │  │  │  ├──0.02 MB (00.05%) ── non-function
>    │  │  │  └──0.01 MB (00.03%) ── function
>    │  │  ├──0.02 MB (00.05%) ── shapes/tree
>    │  │  └──0.01 MB (00.04%) ── sundries [2]
>    │  ├──0.03 MB (00.09%) ── analysis-temporary
>    │  ├──0.03 MB (00.08%) ── other-sundries [2]
>    │  ├──0.02 MB (00.05%) ── objects-extra/slots
>    │  └──0.01 MB (00.03%) ── script-data
>    ├──0.20 MB (00.58%) -- runtime
>    │  ├──0.13 MB (00.36%) ── gc-marker
>    │  ├──0.04 MB (00.10%) ── runtime-object
>    │  ├──0.02 MB (00.05%) ── atoms-table
>    │  ├──0.01 MB (00.03%) ── script-sources
>    │  ├──0.00 MB (00.01%) ── dtoa
>    │  ├──0.00 MB (00.01%) ── stack
>    │  ├──0.00 MB (00.01%) ── temporary
>    │  ├──0.00 MB (00.00%) ── contexts
>    │  ├──0.00 MB (00.00%) ── script-filenames
>    │  ├──0.00 MB (00.00%) ── ion-code
>    │  ├──0.00 MB (00.00%) ── jaeger-code
>    │  ├──0.00 MB (00.00%) ── math-cache
>    │  ├──0.00 MB (00.00%) ── regexp-code
>    │  └──0.00 MB (00.00%) ── unused-code
>    └──0.10 MB (00.29%) -- compartment(web-worker-atoms)
>       ├──0.09 MB (00.26%) -- gc-heap
>       │  ├──0.08 MB (00.25%) ── strings
>       │  └──0.00 MB (00.01%) ── sundries
>       └──0.01 MB (00.03%) ── other-sundries
> 0.59 MB (01.72%) -- worker(resource://gre/modules/wifi_worker.js, 0x43ed4400)
> ├──0.26 MB (00.75%) -- compartment(web-worker)
> │  ├──0.13 MB (00.39%) -- gc-heap
> │  │  ├──0.06 MB (00.17%) ── unused-gc-things [2]
> │  │  ├──0.04 MB (00.11%) -- objects
> │  │  │  ├──0.03 MB (00.08%) ── non-function
> │  │  │  └──0.01 MB (00.03%) ── function
> │  │  ├──0.02 MB (00.05%) ── shapes/tree
> │  │  └──0.02 MB (00.05%) ── sundries [2]
> │  ├──0.06 MB (00.18%) ── analysis-temporary
> │  ├──0.03 MB (00.10%) ── other-sundries [2]
> │  ├──0.02 MB (00.06%) ── objects-extra/slots
> │  └──0.01 MB (00.03%) ── script-data
> ├──0.20 MB (00.58%) -- runtime
> │  ├──0.13 MB (00.36%) ── gc-marker
> │  ├──0.04 MB (00.10%) ── runtime-object
> │  ├──0.02 MB (00.05%) ── atoms-table
> │  ├──0.01 MB (00.03%) ── script-sources
> │  ├──0.00 MB (00.01%) ── dtoa
> │  ├──0.00 MB (00.01%) ── stack
> │  ├──0.00 MB (00.01%) ── temporary
> │  ├──0.00 MB (00.00%) ── contexts
> │  ├──0.00 MB (00.00%) ── script-filenames
> │  ├──0.00 MB (00.00%) ── ion-code
> │  ├──0.00 MB (00.00%) ── jaeger-code
> │  ├──0.00 MB (00.00%) ── math-cache
> │  ├──0.00 MB (00.00%) ── regexp-code
> │  └──0.00 MB (00.00%) ── unused-code
> ├──0.10 MB (00.29%) -- compartment(web-worker-atoms)
> │  ├──0.09 MB (00.26%) -- gc-heap
> │  │  ├──0.09 MB (00.25%) ── strings
> │  │  └──0.00 MB (00.01%) ── sundries
> │  └──0.01 MB (00.03%) ── other-sundries
> └──0.03 MB (00.09%) -- gc-heap
>    ├──0.03 MB (00.09%) ── chunk-admin
>    ├──0.00 MB (00.00%) ── unused-arenas
>    └──0.00 MB (00.00%) ── unused-chunks
There are four relevant workers here:

 * RIL worker (bug 864927)
 * Net worker (bug 864931)
 * wifi worker x2 (this bug)
If no one else is particularly passionate about taking this on, I'd like to.  I used to work with WiFi in a former life, and want to learn more about Gecko guts.  :)
Assignee: nobody → mhabicher
Whiteboard: [MemShrink] → [MemShrink:P1]
blocking-b2g: --- → leo?
We should test again here to see if the memory situation looks as bad now that bug 829482 has landed.
Let me know if you don't know how to test, Mike.
(In reply to Justin Lebar [:jlebar] from comment #4)
>
> Let me know if you don't know how to test, Mike.

Is it just a matter of firing everything up and running get-about-memory.py ?
> Is it just a matter of firing everything up and running get-about-memory.py ?

Yep.
This is on hamachi:

61.33 MB (100.0%) -- explicit
├──28.30 MB (46.14%) -- workers/workers()
│  ├───8.53 MB (13.91%) -- worker(resource://gre/modules/ril_worker.js, 0x4475cc00)
│  │   ├──5.22 MB (08.52%) -- runtime
│  │   │  ├──4.00 MB (06.52%) ── stack
│  │   │  ├──0.66 MB (01.08%) ── temporary
│  │   │  └──0.56 MB (00.91%) ++ (12 tiny)
│  │   ├──1.55 MB (02.53%) -- gc-heap
│  │   │  ├──1.52 MB (02.48%) ── unused-arenas
│  │   │  └──0.03 MB (00.05%) ++ (2 tiny)
│  │   ├──1.50 MB (02.44%) -- compartment(web-worker)
│  │   │  ├──0.78 MB (01.27%) ── analysis-temporary
│  │   │  └──0.72 MB (01.18%) ++ (8 tiny)
│  │   └──0.26 MB (00.42%) ++ compartment(web-worker-atoms)
│  ├───6.65 MB (10.85%) -- worker(resource://gre/modules/wifi_worker.js, 0x44756400)
│  │   ├──4.30 MB (07.02%) -- runtime
│  │   │  ├──4.00 MB (06.52%) ── stack
│  │   │  └──0.30 MB (00.50%) ++ (13 tiny)
│  │   ├──1.70 MB (02.77%) -- gc-heap
│  │   │  ├──1.67 MB (02.72%) ── unused-arenas
│  │   │  └──0.03 MB (00.05%) ++ (2 tiny)
│  │   └──0.65 MB (01.06%) ++ (2 tiny)
│  ├───6.61 MB (10.78%) -- worker(resource://gre/modules/wifi_worker.js, 0x44ad9c00)
│  │   ├──4.30 MB (07.02%) -- runtime
│  │   │  ├──4.00 MB (06.52%) ── stack
│  │   │  └──0.30 MB (00.50%) ++ (13 tiny)
│  │   ├──1.76 MB (02.87%) -- gc-heap
│  │   │  ├──1.73 MB (02.82%) ── unused-arenas
│  │   │  └──0.03 MB (00.05%) ++ (2 tiny)
│  │   └──0.55 MB (00.90%) ++ (2 tiny)
│  └───6.50 MB (10.60%) -- worker(resource://gre/modules/net_worker.js, 0x44f79800)
│      ├──4.24 MB (06.92%) -- runtime
│      │  ├──4.00 MB (06.52%) ── stack
│      │  └──0.24 MB (00.40%) ++ (13 tiny)
│      ├──1.74 MB (02.83%) -- gc-heap
│      │  ├──1.71 MB (02.78%) ── unused-arenas
│      │  └──0.03 MB (00.05%) ++ (2 tiny)
│      └──0.52 MB (00.85%) ++ (2 tiny)
├──13.36 MB (21.78%) -- js-non-window
│  ├───8.15 MB (13.29%) -- compartments
│  │   ├──6.65 MB (10.84%) -- non-window-global
│  │   │  ├──5.00 MB (08.16%) -- compartment([System Principal])
│  │   │  │  ├──1.91 MB (03.11%) -- gc-heap
│  │   │  │  │  ├──1.18 MB (01.92%) ++ (6 tiny)
│  │   │  │  │  └──0.73 MB (01.20%) ++ objects
│  │   │  │  ├──0.98 MB (01.60%) ── script-data
│  │   │  │  ├──0.78 MB (01.28%) ++ string-chars
│  │   │  │  ├──0.72 MB (01.17%) ── analysis-temporary
│  │   │  │  └──0.61 MB (01.00%) ++ (5 tiny)
│  │   │  └──1.64 MB (02.68%) ++ (11 tiny)
│  │   └──1.50 MB (02.45%) -- no-global/compartment(atoms)
│  │      ├──0.99 MB (01.62%) -- string-chars
│  │      │  ├──0.82 MB (01.33%) ── non-huge
│  │      │  └──0.18 MB (00.29%) ── huge/string(length=90725, "data:image//jpeg;base64,//9j//4AAQ...")
│  │      └──0.51 MB (00.83%) ++ (2 tiny)
│  ├───5.11 MB (08.34%) -- runtime
│  │   ├──4.00 MB (06.52%) ── stack
│  │   └──1.11 MB (01.82%) ++ (13 tiny)
│  └───0.09 MB (00.15%) ++ gc-heap
├───6.38 MB (10.40%) ── heap-unclassified
├───5.52 MB (09.00%) -- window-objects
│   ├──4.91 MB (08.01%) -- top(app://system.gaiamobile.org/index.html, id=6)/active
│   │  ├──4.30 MB (07.01%) -- window(app://system.gaiamobile.org/index.html)
│   │  │  ├──2.78 MB (04.53%) -- js/compartment(app://system.gaiamobile.org/index.html)
│   │  │  │  ├──1.05 MB (01.71%) ++ (8 tiny)
│   │  │  │  ├──0.90 MB (01.47%) ++ gc-heap
│   │  │  │  └──0.83 MB (01.35%) ── analysis-temporary
│   │  │  └──1.52 MB (02.49%) ++ (4 tiny)
│   │  └──0.61 MB (00.99%) ++ window(app://keyboard.gaiamobile.org/index.html)
│   └──0.61 MB (01.00%) ++ (2 tiny)
├───3.00 MB (04.90%) -- images
│   ├──3.00 MB (04.90%) -- content
│   │  ├──3.00 MB (04.90%) -- used
│   │  │  ├──2.71 MB (04.43%) ── uncompressed-heap
│   │  │  └──0.29 MB (00.47%) ++ (2 tiny)
│   │  └──0.00 MB (00.00%) ++ unused
│   └──0.00 MB (00.00%) ++ chrome
├───2.19 MB (03.57%) ++ (12 tiny)
├───1.49 MB (02.43%) -- dmd
│   ├──1.24 MB (02.03%) -- stack-traces
│   │  ├──0.79 MB (01.29%) ── unused
│   │  └──0.45 MB (00.73%) ++ (2 tiny)
│   └──0.25 MB (00.41%) ── block-table
└───1.09 MB (01.77%) ── xpti-working-set
Not blocking on this (yet):

* no data showing that a c++ rewrite would definitely reduce memory consumption nor how much

* not clear why js specifically is the memory consumption problem

* not blocking on new perf targets

* late in 1.1 to take wholesale rewrites

* not going to risk product stability on mhabicher's general interest in gecko guts ;)
blocking-b2g: leo? → -
also, is there a bug about "make workers use less memory", if that's a general problem? utilizing works to reduce ui-thread work is a common pattern in Gaia, and something we generally encourage.
(In reply to Dietrich Ayala (:dietrich) from comment #8)
> Not blocking on this (yet):
> 
> * not going to risk product stability on mhabicher's general interest in
> gecko guts ;)

Boo. ;)
FWIW I agree that this isn't a blocker.

> * not clear why js specifically is the memory consumption problem

But it /is/ pretty clear that JS specifically is the memory-consumption problem, if you look at the memory report in comment 0.  The biggest offender is "unused-arenas", which is memory that we could but aren't decommitting (bug 829482).  That's followed by other JS miscellanea.

gc-objects, which corresponds to "data stored by these workers", is a paltry 0.03mb.  That's the only overhead we'd have if we rewrote this in C++.

We had a long newsgroup thread about this and the conclusion was that workers aren't going to become particularly memory-efficient in the near future.  I really appreciate that mikeh stepped up to look at this important bug.
(In reply to Mike Habicher [:mikeh] from comment #7)
> This is on hamachi:
> 
> 61.33 MB (100.0%) -- explicit
> ├──28.30 MB (46.14%) -- workers/workers()
> │  ├───8.53 MB (13.91%) -- worker(resource://gre/modules/ril_worker.js,
> 0x4475cc00)
> │  │   ├──5.22 MB (08.52%) -- runtime
> │  │   │  ├──4.00 MB (06.52%) ── stack

This is a debug build, right?  The "runtime/stack" measurement for debug builds overmeasures.  In a debug build this amount is close to zero, e.g. in comment zero.

I'd love to know if the fix to bug 829482 did reduce the "unused-arenas" size.
> The "runtime/stack" measurement for debug builds overmeasures.

iirc that fix isn't on b2g18.
Status: NEW → ASSIGNED
I haven't had a chance to get into this yet, and it looks like I won't any time soon. If someone else wants this, go ahead.
Assignee: mhabicher → nobody
Status: ASSIGNED → NEW
Attached patch wip (obsolete) — Splinter Review
This wip only gets rid of one of the 2 workers: the "event" worker, used to poll the wifi_wait_for_event() method. I started with this one because it's slightly simpler and I wanted to get an idea of the gains if any.

The results I see with a setup that consists on connecting to a non-authenticated wifi access point are too good to be true (~2.5 MB of win) so I'd like to get independent testing by others.

Blake, the code clearly still needs quite some work, but let me know of anything you see there that you don't like!
Assignee: nobody → fabrice
Attachment #789205 - Flags: feedback?(mrbkap)
Comment on attachment 789205 [details] [diff] [review]
wip

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

Dynamic library loading might be hard on the memory in JS, but it's really hard on the eyes in C++! This looks like a good start to me, overall.

::: dom/wifi/WifiProxyService.cpp
@@ +15,5 @@
> +using namespace mozilla;
> +
> +StaticRefPtr<WifiProxyService> gWifiProxyService;
> +
> +WpaSupplicant* gWpaSupplicant;

Should the two variables here be static? Also, comments explaining what thread they should be accessed on would be very useful.

@@ +37,5 @@
> +// Runnable used to call WaitForEvent on the event thread.
> +class EventRunnable : public nsRunnable
> +{
> +public:
> +  EventRunnable() {};

Nit: no ; needed.

Please assert what thread you expect in the constructor and in Run (also in WifiEventDispatcher).

::: dom/wifi/WifiProxyService.h
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef __WIFIPROXYSERVICE_H__

#define __WIFIPROXYSERVICE_H__

@@ +16,5 @@
> +
> +  static already_AddRefed<WifiProxyService>
> +  FactoryCreate();
> +
> +  void dispatchWifiEvent(nsACString& aEvent);

Nit: C++ tends to have initial capitals on method names (so: DispatchWifiEvent).

::: dom/wifi/WifiUtils.h
@@ +6,5 @@
> + * Abstraction on top of the wifi support from libhardware_legacy that we
> + * use to talk to the wpa_supplicant.
> + */
> +
> +#ifndef __WIFIUTILS_H__

#define __WIFIUTILS_H__

@@ +19,5 @@
> +
> +// Intentionally not trying to dlclose() this handle. That's playing
> +// Russian roulette with security bugs.
> +static void* sWifiLib;
> +static PRCallOnceType sInitWifiLib;

Declaring statics like this in .h files is really odd. Shouldn't these be in a cpp file somewhere?

@@ +25,5 @@
> +static PRStatus
> +InitWifiLib()
> +{
> +  sWifiLib = dlopen("/system/lib/libhardware_legacy.so", RTLD_LAZY);
> +  // We might fail to open the camera lib. That's OK.

s/camera/wifi/ :)

@@ +76,5 @@
> +      aEvent = buffer;
> +      //XXX: convert utf-8 to wstring. can't get it to work yet...
> +      /*PRUnichar wideString[ret + 1];
> +      ConvertUTF8toUTF16 converter(wideString);
> +      converter.write(buffer, ret);

We need to be pretty careful here about how we deal with invalid UTF8 (see bug 828782). It looks like the converter here will bail out, currently, which won't work for our use-cases.

@@ +110,5 @@
> +      mImpl = new ICSWpaSupplicantImpl();
> +    } else {
> +      mImpl = new JBWpaSupplicantImpl();
> +    }
> +  };

Nit: Here and below, no ; after method definitions.

@@ +121,5 @@
> +    mImpl->WaitForEvent(aEvent);
> +  }
> +
> +private:
> +  WpaSupplicantImpl* mImpl;

I'd prefer an AutoPtr<> here (and no manual deleting).

::: dom/wifi/moz.build
@@ +33,5 @@
>      'libhardware_legacy.js',
>      'wifi_worker.js',
>  ]
>  
> +#ifdef MOZ_WIDGET_GONK

I don't think this is the right way to do conditionals in moz.build files. It looks like this might want to be:
if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':

::: dom/wifi/nsIWifiService.idl
@@ +6,5 @@
> +#include "nsIEventTarget.idl"
> +#include "nsIDOMEvent.idl"
> +
> +[scriptable, uuid(f894e459-335f-4558-91dc-ffbd72f7da5f)]
> +interface nsIWifiProxyListener : nsISupports {

The naming here is a little confusing. Why is this a Proxy listener?
Attachment #789205 - Flags: feedback?(mrbkap) → feedback+
Attached patch patch v1 (obsolete) — Splinter Review
No wifi workers anymore in this patch! I traded them for a few MACROS...

I only implemented the commands that I saw being actually used, and I'm not sure yet to cover all our use cases.
The JS code supported GB, this patch does not as we don't support GB in general anymore. I made as few changes to WifiWorker.js as possible, and this reflects in the the Results dictionnary that is actually a union of all the results that we can send. We could split it up, especially for do_dhcp_request.

Try run at: https://tbpl.mozilla.org/?tree=Try&rev=556e5ac46800
Attachment #789205 - Attachment is obsolete: true
Attachment #791733 - Flags: review?(mrbkap)
Comment on attachment 791733 [details] [diff] [review]
patch v1

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

::: dom/wifi/WifiProxyService.cpp
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsServiceManagerUtils.h"
> +#include "WifiProxyService.h"

Include WifiProxyService.h first, to ensure it can build on its own.

@@ +50,5 @@
> +  NS_IMETHOD Run()
> +  {
> +    nsAutoString event;
> +    gWpaSupplicant->WaitForEvent(event);
> +    if (event.Length()) {

!event.IsEmpty()

@@ +103,5 @@
> +WifiProxyService::WifiProxyService()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!gWifiProxyService);
> +  printf_stderr("WifiProxyService::WifiProxyService");

Remove this before landing

@@ +133,5 @@
> +  return service.forget();
> +}
> +
> +NS_IMETHODIMP
> +WifiProxyService::Start(nsIWifiEventListener *listener)

* to the left; aListener

@@ +138,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(listener);
> +
> +  printf_stderr("WifiProxyService::Start\n");

Ditto

@@ +197,5 @@
> +    return;                               \
> +  }
> +
> +#define SET_STRING(prop)                                                      \
> +  JSString *str##prop = JS_InternUCStringN(cx, (jschar *)aOptions.prop.get(), \

No c-style casts; * to the left

@@ +214,5 @@
> +
> +  mozilla::AutoSafeJSContext cx;
> +
> +  JS::Rooted<JSObject*> o(cx, JS_NewObject(cx, nullptr, nullptr, nullptr));
> +  JS::Rooted<JS::Value> v(cx, INT_TO_JSVAL(aOptions.id));

This really sucks.

::: dom/wifi/WifiProxyService.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef __WIFIPROXYSERVICE_H__
> +#define __WIFIPROXYSERVICE_H__

WifiProxyService_h per the style guide

@@ +9,5 @@
> +#include "nsCOMPtr.h"
> +#include "nsThread.h"
> +#include "DictionaryHelpers.h"
> +
> +using namespace mozilla::idl;

No 'using's in headers

@@ +11,5 @@
> +#include "DictionaryHelpers.h"
> +
> +using namespace mozilla::idl;
> +
> +class WifiProxyService MOZ_FINAL : public nsIWifiProxyService

Namespace this class

::: dom/wifi/WifiUtils.cpp
@@ +35,5 @@
> +// https://mxr.mozilla.org/mozilla-central/source/js/src/vm/CharacterEncoding.cpp#231
> +
> +static const uint32_t REPLACE_UTF8 = 0xFFFD;
> +
> +void LossyConvertUTF8toUTF16(const char* aInput, uint32_t aLength, nsAString& aOut)

Surely we already have code to do that in xpcom! And why "lossy"?

@@ +403,5 @@
> +WpaSupplicant::SdkVersion()
> +{
> +  char propVersion[PROPERTY_VALUE_MAX];
> +  property_get("ro.build.version.sdk", propVersion, "0");
> +  return atoi(propVersion);

atoi is strongly frowned upon, aiui

::: dom/wifi/WifiUtils.h
@@ +7,5 @@
> + * use to talk to the wpa_supplicant.
> + */
> +
> +#ifndef __WIFIUTILS_H__
> +#define __WIFIUTILS_H__

Ditto

@@ +14,5 @@
> +#include "nsAutoPtr.h"
> +#include "DictionaryHelpers.h"
> +#include "NetUtils.h"
> +
> +using namespace mozilla::idl;

Ditto

@@ +23,5 @@
> +class WpaSupplicantImpl
> +{
> +public:
> +  virtual int32_t
> +    do_wifi_wait_for_event(const char *iface, char *buf, size_t len) = 0; // ICS != JB

CamelCase

::: js/xpconnect/src/dictionary_helper_gen.conf
@@ +13,5 @@
>       [ 'CameraRecordingOptions', 'nsIDOMCameraManager.idl' ],
>       [ 'SmsThreadListItem', 'nsIMobileMessageCallback.idl' ],
> +     [ 'MmsAttachment', 'nsIDOMMozMmsMessage.idl' ],
> +     [ 'WifiCommandOptions', 'nsIWifiService.idl' ],
> +     [ 'WifiResultOptions', 'nsIWifiService.idl' ]

We're going to remove this in the relatively near future, so it'd be nice to avoid adding new XPIDL dictionaries.
Attachment #791733 - Flags: review?(mrbkap)
You said that the memory win was ~2.5mb for removing the one worker.  What does it look like now that you've removed both of them?
Attached patch patch v2 (obsolete) — Splinter Review
I addressed most of Ms2ger comments except the following:
- I still use atoi(). Happy to switch to something else, along with the ~ 200 other uses we have in the tree.
- I didn't camel cased the functions that are using underscores: we are mostly wrapping C functions from android and I find it much easier to keep them this way. Let's see how mrbkap feels on that.

Justin, for some unknown reason I can't run get_about_memory.py to work on my builds : the homescreen app is dies during the memory report (nothing in gdb), and no .json.gz is created in /data/local/tmp/memory-reports. I'll investigate more tomorrow.
Attachment #791733 - Attachment is obsolete: true
> Justin, for some unknown reason I can't run get_about_memory.py to work on my builds : 
> the homescreen app is dies during the memory report (nothing in gdb)

That's very odd.  Is it OOMing?  (Check dmesg.)  Let me know if you need me to help.
Attached patch patch v3 (obsolete) — Splinter Review
Updated to use webidl dictionaries.
Attachment #791870 - Attachment is obsolete: true
Attachment #793018 - Flags: review?(mrbkap)
Comment on attachment 793018 [details] [diff] [review]
patch v3

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

I'd like an answer to some of my questions before stamping + but there's nothing major in my comments.

::: dom/wifi/NetUtils.cpp
@@ +42,5 @@
> +NetUtils::SdkVersion()
> +{
> +  char propVersion[PROPERTY_VALUE_MAX];
> +  property_get("ro.build.version.sdk", propVersion, "0");
> +  int32_t version = strtol(propVersion, nullptr, 10);

Is it worth asserting that propVersion was actually a number?

@@ +50,5 @@
> +// Defines a function type with the right arguments and return type.
> +#define DEFINE_DLFUNC(name, ret, args...) typedef ret (*FUNC##name)(args);
> +
> +// Set up a dlsymed function ready to use.
> +#define USE_DLFUNC(name)                                           \

Would you mind aligning the \s in column 79?

::: dom/wifi/WifiProxyService.cpp
@@ +38,5 @@
> +    return NS_OK;
> +  }
> +
> +private:
> +  nsAutoString mEvent;

We tend to not use nsAutoString for heap-allocated objects. I'd prefer that this be nsString.

@@ +46,5 @@
> +class EventRunnable : public nsRunnable
> +{
> +public:
> +  EventRunnable() {
> +    MOZ_ASSERT(NS_IsMainThread());

Nit: You're mixing bracing styles between the constructor and Run.

@@ +51,5 @@
> +  }
> +
> +  NS_IMETHOD Run()
> +  {
> +    nsAutoString event;

Please MOZ_ASSERT(!NS_IsMainThread()); here.

@@ +66,5 @@
> +class WifiResultDispatcher : public nsRunnable
> +{
> +public:
> +  WifiResultDispatcher(WifiResultOptions& aResult) {
> +    // XXX: is there a better way to copy webidl dictionnaries?

Bracing style again... Also assert that we're not on the main thread.

@@ +91,5 @@
> +    COPY_FIELD(mGateway)
> +    COPY_FIELD(mDns1)
> +    COPY_FIELD(mDns2)
> +    COPY_FIELD(mServer)
> +  }

#undef COPY_FIELD

::: dom/wifi/WifiUtils.cpp
@@ +34,5 @@
> +}
> +
> +// This is the same algorithm as in InflateUTF8StringToBuffer with Copy and
> +// while ignoring invalids.
> +// https://mxr.mozilla.org/mozilla-central/source/js/src/vm/CharacterEncoding.cpp#231

Please file a followup bug on figuring out how to share the code between these two places. Also, add a comment in CharacterEncoding.cpp to point here in case someone changes that algorithm.

@@ +121,5 @@
> +
> +// Helper to check we have loaded the hardware shared library.
> +#define CHECK_HWLIB(ret)                                  \
> +  void* hwLib = GetWifiLibHandle();                       \
> +    if (!hwLib) {                                         \

Nit: Indentation seems off after the first line.

@@ +136,5 @@
> +  if (!name) {                                                     \
> +    MOZ_ASSERT("Symbol not found in libhardware_legacy : " #name); \
> +  }
> +
> +#define DEFAULT_IMPL(name, ret, args...) \

Would it be possible to refactor these macros so you don't have two copies of DEFINE_DLFUNC, USE_DLFUNC and DEFAULT_IMPL?

@@ +153,5 @@
> +  DEFAULT_IMPL(wifi_stop_supplicant, int32_t, )
> +
> +  DEFINE_DLFUNC(wifi_wait_for_event, int32_t, char*, size_t)
> +  int32_t
> +  do_wifi_wait_for_event(const char *iface, char *buf, size_t len) {

Nit (here and below): I don't see a reason to split the return value on its own line when we're in a class definition.

@@ +160,5 @@
> +  }
> +
> +  DEFINE_DLFUNC(wifi_command, int32_t, const char*, char*, size_t*)
> +  int32_t
> +  do_wifi_command(const char* iface, const char* cmd, char* buff, size_t* len) {

buff -> buf

@@ +272,5 @@
> +
> +  // Always correlate the opaque ids.
> +  aResult.mId = aOptions.mId;
> +
> +  if (aOptions.mCmd.EqualsLiteral("property_get")) {

Please file a followup on removing this -- we should just call property_get and property_set directly in WifiWorker.js

@@ +360,5 @@
> +    aResult.mMask = MakeMask(prefixLength);
> +
> +    uint32_t inet4; // only support IPv4 for now.
> +
> +#define INET_PTON(var, field) inet_pton(AF_INET, var, &inet4); \

Nit: I'd use PR_BEGIN_MACRO/PR_END_MACRO and split it up on 4 lines.

@@ +399,5 @@
> +
> +// Checks the buffer and do the utf processing.
> +void
> +WpaSupplicant::CheckBuffer(char* buffer, int32_t length,
> +                                      nsAString& aEvent)

Nit: the nsAString on the second line should align with the 'c' in "char* buffer".

@@ +412,5 @@
> +WpaSupplicant::SdkVersion()
> +{
> +  char propVersion[PROPERTY_VALUE_MAX];
> +  property_get("ro.build.version.sdk", propVersion, "0");
> +  int32_t version = strtol(propVersion, nullptr, 10);

This really wants to be refactored with the other SdkVersion.

::: dom/wifi/WifiUtils.h
@@ +47,5 @@
> +
> +#define COPY_FIELD(prop) prop = aOther.prop;
> +    COPY_FIELD(mId)
> +    COPY_FIELD(mCmd)
> +    COPY_OPT_FIELD(mRequest, NS_LITERAL_STRING(""))

NS_LITERAL_STRING("") -> EmptyString()

@@ +60,5 @@
> +    COPY_OPT_FIELD(mValue, NS_LITERAL_STRING(""))
> +    COPY_OPT_FIELD(mDefaultValue, NS_LITERAL_STRING(""))
> +  }
> +
> +// All the fields, not Optional<> anymore to get copy constructors.

Nit: this comment should be indented.

@@ +83,5 @@
> +class WpaSupplicantImpl
> +{
> +public:
> +  virtual int32_t
> +    do_wifi_wait_for_event(const char *iface, char *buf, size_t len) = 0; // ICS != JB

Nit: Funny indentation.

::: dom/wifi/WifiWorker.js
@@ +101,5 @@
> +      }
> +    },
> +
> +    onCommand: function(event) {
> +      //dump("ZZZ wifiListener::onCommand #" + event.id + " " + uneval(event));

Remove this entirely before checkin.
Attachment #793018 - Flags: review?(mrbkap)
Attached patch patch v4Splinter Review
I addressed all the review comments afaict (just need to file the followups).

About:

> Is it worth asserting that propVersion was actually a number?

I don't think we should do anything there. If the property is incorrect, something is deeply broken anyway and there's not much we can do. We'll return 0 and wifi will just not work.
Attachment #793018 - Attachment is obsolete: true
Attachment #795116 - Flags: review?(mrbkap)
Depends on: 909065
Depends on: 909066
Comment on attachment 795116 [details] [diff] [review]
patch v4

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

One small comment. r=me with it addressed.

::: dom/wifi/WifiUtils.h
@@ +58,5 @@
> +    COPY_OPT_FIELD(mDns2, 0)
> +    COPY_OPT_FIELD(mKey, EmptyString())
> +    COPY_OPT_FIELD(mValue, EmptyString())
> +    COPY_OPT_FIELD(mDefaultValue, EmptyString())
> +  }

Please #undef the two macros here.
Attachment #795116 - Flags: review?(mrbkap) → review+
Attached file baseline memory report
Here's the memory report without the patch, on a tcl device running current gaia master and b2g-inbound, while being connected to Mozilla-Guest wifi network.
Same conditions for this report, but with this patch and the small one from bug 909065.

Use the diff functionality of about:memory, I see -3.79 MB of explicit memory in the main process, and -3.10 MB of RSS. The summary looks like:

-1.42 MB ── heap-allocated
-0.98 MB ── heap-committed
   1.80% ── heap-overhead-ratio
-0.00 MB ── js-main-runtime-temporary-peak
    -261 ── page-faults-hard
  -6,588 ── page-faults-soft
-3.34 MB ── resident
-3.40 MB ── resident-unique
-6.21 MB ── vsize
> -3.34 MB ── resident
> -3.40 MB ── resident-unique
> -6.21 MB ── vsize

Nice!
https://hg.mozilla.org/mozilla-central/rev/f3c5e1c6ab9e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.