968132 - Calculate the hard limit for decoded image buffer instead of a hard-coded preference

Status: RESOLVED WONTFIX

(Core :: Graphics: ImageLib, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

By @milan's suggestion in Bug 878577 comment #8, it'll be better to provide a reference design by guessing the buffer size limit at runtime.

Comment 1

[Fredrik Lanker [:lanker]]

Attachment: Bug-968132-Calculate-the-hard-limit-for-decoded-imag.patch

Changes the preference to specify a percentage of total system memory

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8526752 [details] [diff] [review]
Bug-968132-Calculate-the-hard-limit-for-decoded-imag.patch

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

I'll defer to Seth, but this doesn't quite feel right.  Since we have the device specific preferences that OEMs can set, it is easy enough to set the original value in KB to something different for the larger devices.  25% may be too little or too much for anything but 256mb devices - would we really want 128mb on 512mb devices, for instance?  If we were to decide we want the percentage of the system memory, then I'd rather see a separate, new, preference, which, when set to 0, uses the old value (in kb) and when set to > 0, computes it as a percentage.  But I'm not sure we actually want to move away from what we have, my comment that may have started this non-withstanding.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Hi Fredrik, thanks for working on this!

Using the percentage of total system memory sounds like a good start. The only concern I have is that you are using a linear function here. For 1GB platform 25% seems sufficient, but for 128MB platform we might need up to 50% to ensure the browsing experience on image-intensive websites/apps.

In this bug we are trying to avoid do the preference adjustment on every product/platform and the hard limit we have now is too conservative to a memory-rich platform. My suggestion is we can still provide a memory limit in KB according to our most-memory-constraint platform as a minimum guarantee and use a percentage if we got more memory.

The equation will look like this:

  limit_we_choose = max([pre-configured memory size], (total_memory * [pre-configured percentage]))

Comment 4

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8526752 [details] [diff] [review]
Bug-968132-Calculate-the-hard-limit-for-decoded-imag.patch

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

I'm r-'ing this because the DiscardTracker-based hard limit is about to go away totally. (Indeed, the entire DiscardTracker is being removed.)

The SurfaceCache (which is replacing the DiscardTracker) already has a hard limit that works exactly in this way, as a percentage of main memory.

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #3)
>   limit_we_choose = max([pre-configured memory size], (total_memory *
> [pre-configured percentage]))

The SurfaceCache uses min([maximum limit set by pref], (total_memory * [percentage set by pref])).

Comment 6

[Seth Fowler [:seth] [:s2h]]

Fredrik, I do want to thank you for working on this. It's really unfortunate timing that DiscardTracker is getting removed just as you posted the patch. Thanks so much for taking the time to look into making this happen!