On Tue, Nov 5, 2013 at 1:35 PM, Sriram Ramasubramanian < [email protected]> wrote:
> 5. We need some kind of chart / tool that monitors memory usage. I think > we have one, but we don’t pay enough attention as Eideticker or other tests. > We have https://areweslimyet.com/mobile/. However, this doesn't provide details into where that memory is allocated, like what running Fennec with `monitor` attached would do. I wonder if we should look into some other cases to add to this site, since I think it was designed with mostly Gecko memory usage in mind. I'm also cc'ing njn, in case he's not on mobile-firefox-dev, because I think he would be interested in this memory discussion. Margaret On Tue, Nov 5, 2013 at 1:35 PM, Sriram Ramasubramanian < [email protected]> wrote: > To summarize: > 1. Prefer not to use inner-classes. If used, please try to make them > static as much as possible. > > 2. Inner classes shouldn’t be holding references to heavy objects. > Especially if its extending an AsyncTask. > > A class example would be where a View triggers an AsyncTask to update an > image on it. Instead of holding a strong reference, hold a weak reference. > Once the job completes, if the view is still there, use it. If not, that > job was waste. > > 3. It’s better to create one private (preferably static) inner class > member (and mark it final), that can handle different callbacks. This is a > neat way of coding recommended everywhere. > > class SomeView { > private static final Callbacks implements OnClickListener, > OnSelectedListener, Gecko.OnTabsListener { … } > } > > 4. If you are using anonymous callbacks, see if they are better used as > final members. On key fix of rnewman removed many OnFaviconLoadedListeners, > that weren’t dependent on View’s member variables. It did something like, > “once loaded, tell Tabs that it’s loaded”. A single static member can be > assigned to multiple class instances — in such cases. > > 5. We need some kind of chart / tool that monitors memory usage. I think > we have one, but we don’t pay enough attention as Eideticker or other tests. > > 6. Every object should have proper cleanups. When a view detaches from a > window, it should unregister from Tabs, and so on. > > 7. Our code architecture needs proper documentation that is accessible > from the web. That way we know, if some other class does the same job or > not, and we won’t indulge in re-inventing the wheel. I remember, at one > point, we had three inner classes named “LoadFaviconTask” in 3 different > locations. > > 8. Telemetry should be able to track the memory used by our application, > and send back pings to the server. That way, we will know “how many tabs > and how many MB” for different devices. That could help us identify things > like, “why is Nexus S using lesser memory than a S2?”, as that would be a > small screen size change causing us using 512px vs 1024px for image width > (that’s just an example). > > - Sriram. > > > On Nov 4, 2013, at 1:39 PM, Richard Newman <[email protected]> wrote: > > > There are a few abstract observations that resulted from the concrete > problems in my last message. They essentially boil down to: how can we > protect ourselves from our own code? Or, phrased differently: how can I > avoid introducing bugs in the future? > > > > I am very keen to hear ideas on how to shift the odds in our favor. If > we can gather some recommendations that we agree on, I'll write them up > (and add them to the reviewer guidelines). > > > > > > Firstly, we ended up with a bunch of OOMs on mid-to-high-end devices > because we made some valid assumptions on smaller devices. > > > > On an older RAZR device, with 248-pixel-wide thumbnails, jumping to the > next power of two is fine. Arguably on *any* device, you don't want to take > a screenshot smaller than 256 or 512px. > > > > Even if it blows up one power too far, it's not 2.3MB. And the larger > the screenshot, the prettier it is when scaled down. That decision was > valid, right up until we crossed the 512px column width threshold. > > > > I also heard opinions in IRC that this size didn't matter, because of > course that was just the capture size, which any device can do for just a > moment, right? That assumption wasn't borne out by the code: we store the > full-size image (PNG-compressed) in the DB and extract it on launch, and we > hold a reference to the full-size bitmap rather than scaling it down and > throwing away the larger version. > > > > We make decisions like this all the time: buffer sizes, batch sizes, > preferred image dimensions, maximum cache thresholds. As far as I know, we > don't have an institutionalized mechanism for forcing ourselves to be aware > of these decisions. Imagine if we had a test that verified that our > thumbnails were never more than a sanity threshold -- in 2010 that sanity > threshold would have been maybe 500KB, and each time that test failed > (e.g., when we first hit xhdpi devices), with a nice descriptive error > message, we'd be forced to reassess whether we should raise the limit, > change our approach, or do something else. > > > > When you write or review code that contains an explicit or systemic > limit, think about whether you want to add a test, or a runtime check, to > call attention to a broken assumption. If nothing else, this will force you > to think about the implicit assumptions you're making. Perhaps the right > outcome is a telemetry probe that reports cache fullness, or cache resize > events. Perhaps it's a test that just checks the assumption. At the very > least it ought to be a comment. > > > > (And I suppose the limit applies in the other direction, too: on 2015's > 16GB handsets, is today's 512KB cache size a good tradeoff?) > > > > > > > > Secondly, it doesn't seem like we're paying as much attention as we > should to memory usage… else we'd have spotted this before on our > daily-driver phones. I strongly encourage people to run Fennec with > `monitor` attached. You can watch incremental memory allocations, and take > heap snapshots and analyze them. Hell, I learn a lot just by watching the > full adb log scroll by. > > > > (See <https://github.com/rnewman/android-quiet-logs> if you want to do > something noisier but more valuable than `fgrep Gecko`.) > > > > I won't go so far as to say that this should be a pre-commit hook, or > even a mandatory review step, but I know I'm going to keep this in mind for > my next review or patch. > > > > I filed a meta bug off which to hang bugs that result from memory > analysis. Next time you're testing by hand, and you have a non-empty > profile, try grabbing a memory snapshot and familiarizing yourself with the > interface. Maybe you find that some feature is holding on to a reference > and leaking memory… or maybe you watch the incremental allocator and wonder > why it cost two megs to open a menu. File a bug! > > > > > > > > Thirdly, our code architecture is difficult to understand (e.g., with > favicon and thumbnail logic spanning tabs, listeners, the favicon system > itself, the DB, bits of Gecko, …). That leads to incremental changes that > orphan code: it's cargo-culted forward between revisions. It was enormously > valuable to me to have pointers to bugs explaining *why* we did one thing > and not another. Fortunately I have an obstreperous and questioning > personality! > > > > It also leaves a lot of opportunity for memory leaks -- a foo holds on > to a bar, the bar holds on to a map, the map contains tasks, the tasks hold > on to … . When you need six buffers open to figure out who owns what, > you're in trouble. > > > > I'd love for us to spend a little of our copious free time pushing > towards simplicity -- pushing related logic together, reducing coupling, > avoiding the temptation to just fling another runnable onto a background > thread and mutate another instance's member variables. It'll pay off in the > long run. > > > > > > > > Thoughts? > > > > -R > > _______________________________________________ > > mobile-firefox-dev mailing list > > [email protected] > > https://mail.mozilla.org/listinfo/mobile-firefox-dev > > _______________________________________________ > mobile-firefox-dev mailing list > [email protected] > https://mail.mozilla.org/listinfo/mobile-firefox-dev >
_______________________________________________ mobile-firefox-dev mailing list [email protected] https://mail.mozilla.org/listinfo/mobile-firefox-dev
