Closed Bug 1037302 Opened 6 years ago Closed 6 years ago

Avoid excess string creation in WifiCommand.jsm's getConnectionInfoICS()

Categories

(Firefox OS Graveyard :: Wifi, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

When wi-fi is running, command.getConnectionInfoICS() in WifiCommand.jsm
receives a string like this every five seconds:

> "RSSI=-50\nLINKSPEED=54\nNOISE=9999\nFREQUENCY=0"

It extracts the RSSI and LINKSPEED values. It does this by splitting the string
at newlines to get the "key=value" sub-strings, and then splitting each of
those at '=' to get separate keys and values, and then converting the key to
upper-case, and then seeing if the upper-cased key is one we care about. For
the example input above, it creates 16 strings.

These can add up to non-trivial numbers of strings after a while, and while
they do get collected eventually it can take a while. For example, after about
49 minutes:

> ├─────47,200 B (00.11%) -- string(length=9, copies=1180, "FREQUENCY")
> ├─────47,200 B (00.11%) -- string(length=9, copies=1180, "LINKSPEED")
> ├─────37,760 B (00.09%) -- string(length=4, copies=1180, "RSSI")
> ├─────37,760 B (00.09%) -- string(length=5, copies=1180, "NOISE")
> ├─────18,880 B (00.04%) ── string(length=10, copies=590, "NOISE=9999")/gc-heap
> ├─────18,880 B (00.04%) ── string(length=11, copies=590, "FREQUENCY=0")/gc-heap
> ├─────18,880 B (00.04%) ── string(length=4, copies=590, "9999")/gc-heap
> ├─────15,744 B (00.04%) ── string(length=8, copies=492, "RSSI=-47")/gc-heap
> ├──────9,440 B (00.02%) ── string(length=12, copies=590, "LINKSPEED=54")/gc-heap
> ├──────7,872 B (00.02%) ── string(length=3, copies=492, "-47")/gc-heap
> ├──────7,872 B (00.02%) ── string(length=44, copies=492, "RSSI=-47/nLINKSPEED=54/nNOISE=9999/nFREQUENC")/gc-heap
> ├──────2,336 B (00.01%) ── string(length=8, copies=73, "RSSI=-48")/gc-heap
> ├──────1,168 B (00.00%) ── string(length=3, copies=73, "-48")/gc-heap
> ├──────1,168 B (00.00%) ── string(length=44, copies=73, "RSSI=-48/nLINKSPEED=54/nNOISE=9999/nFREQUENC")/gc-heap

That's over 264 KiB. We can do better.
I'm confident this new code is correct. Having said that, do we have tests that
would fail if I introduced a bug with this change?
Attachment #8454238 - Flags: review?(hchang)
Whiteboard: [MemShrink]
(In reply to Nicholas Nethercote [:njn] from comment #1)
> Created attachment 8454238 [details] [diff] [review]
> Avoid excess string creation in WifiCommand.jsm's getConnectionInfoICS()
> 
> I'm confident this new code is correct. Having said that, do we have tests
> that
> would fail if I introduced a bug with this change?

Since the callback value will be used by gaia, UI would display wrong 
wifi signal strength if |rval| is not parsed properly. 

By the way, we have enabled wifi on ICS emulator but unfortunately
the test driver doesn't reply SIGNAL_POLL correctly. Not sure if
any gaia ui/unit tests covers this.
Comment on attachment 8454238 [details] [diff] [review]
Avoid excess string creation in WifiCommand.jsm's getConnectionInfoICS()

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

Pretty nice and helpful work! Thanks! 
Maybe we should also review all the parsing code 
in wifi to avoid such issue.

Thanks!

::: dom/wifi/WifiCommand.jsm
@@ +241,5 @@
>    };
>  
> +  command.infoKeys = [{regexp: /RSSI=/i,      prop: 'rssi'},
> +                      {regexp: /LINKSPEED=/i, prop: 'linkspeed'}];
> +

Is there any reason to export |infoKeys|? I would move it to be function scoped of command.getConnectionInfoICS or a private variable to WifiCommand.
Attachment #8454238 - Flags: review?(hchang) → review+
> Maybe we should also review all the parsing code 
> in wifi to avoid such issue.

This is the only case that has shown up as significant in my profiling.

> Is there any reason to export |infoKeys|? I would move it to be function
> scoped of command.getConnectionInfoICS or a private variable to WifiCommand.

Putting it within command.getConnectionInfoICS is bad because then we'll end up creating 1 array, 2 ordinary objects and 2 regexp objects per call. But making it a private variable in WifiCommand is a good idea.
https://hg.mozilla.org/mozilla-central/rev/8112c0230130
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Nicholas Nethercote [:njn] from comment #4)
> > Maybe we should also review all the parsing code 
> > in wifi to avoid such issue.
> 
> This is the only case that has shown up as significant in my profiling.
> 
> > Is there any reason to export |infoKeys|? I would move it to be function
> > scoped of command.getConnectionInfoICS or a private variable to WifiCommand.
> 
> Putting it within command.getConnectionInfoICS is bad because then we'll end
> up creating 1 array, 2 ordinary objects and 2 regexp objects per call. But
> making it a private variable in WifiCommand is a good idea.

Actually we can avoid creating object per call but still make it 
function scoped by doing this (a little lousy though :p)

command.getConnectionInfoICS = (function() {
  let infoKeys = [...];
  return function(callback) {
    // ...
  }
})();
You need to log in before you can comment on or make changes to this bug.