Closed
Bug 1037302
Opened 10 years ago
Closed 10 years ago
Avoid excess string creation in WifiCommand.jsm's getConnectionInfoICS()
Categories
(Firefox OS Graveyard :: Wifi, defect)
Firefox OS Graveyard
Wifi
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
(Whiteboard: [MemShrink])
Attachments
(1 file)
2.62 KB,
patch
|
hchang
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: [MemShrink]
Comment 2•10 years ago
|
||
(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 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
> 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.
Assignee | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/8112c0230130
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8112c0230130
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 7•10 years ago
|
||
(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.
Description
•