On Tue, Nov 5, 2013 at 1:35 PM, Sriram Ramasubramanian
<[email protected] <mailto:[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] <mailto:[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]
<mailto:[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]
<mailto:[email protected]>
> https://mail.mozilla.org/listinfo/mobile-firefox-dev
_______________________________________________
mobile-firefox-dev mailing list
[email protected] <mailto:[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
