[webkit-reviews] review denied: [Bug 89111] Heap::shouldCollect() should return true when system memory is low : [Attachment 147821] Last try

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Yong Li <yoli at rim.com>'s request
for review:
Bug 89111: Heap::shouldCollect() should return true when system memory is low
https://bugs.webkit.org/show_bug.cgi?id=89111

Attachment 147821: Last try
https://bugs.webkit.org/attachment.cgi?id=147821&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
Thinking theoretically, I see how it's elegant to GC sooner if the system is
running out of memory.

However, in practice, this kind of policy has had very surprising negative
consequences. Namely:

- Many systems always claim to be "running out of memory" because they
aggressively fill RAM with virtual memory caches. On these systems, the "GC
sooner" policy collects way too often, and O(n^2) at the limit. This has
happened before.

- Some systems dynamically free up memory by killing unneeded processes,
evicting VM caches, or paging -- but only if we request new VM pages first. On
these systems, the "GC sooner" policy substantially reduces GC performance for
no practical gain.

- Some systems respond to memory pressure by swapping portions of the heap into
compressed or paged storage. On these systems, the "GC sooner" policy makes low
memory conditions orders of magnitude worse by constantly swapping the GC heap
back into cached/uncompressed/unpaged storage, evicting everything else. This
has happened before.

- Some systems can only tell you if you're "running out of memory" by running
an expensive operation. On these systems, the "GC sooner" policy is very slow
because it performs an expensive operation very frequently.

These data points make me think that this patch would be a serious performance
regression. On the meta level, these data points make me very suspicious of any
GC policy change that isn't accompanied by a measurable benefit in some
benchmark or known user scenario on a reasonable variety of systems. The
outcome is too unpredictable for us to operate on theory alone.

Since this patch is Blackberry-only, and doesn't have any data to prove its
benefit, I'm marking it r-.

I would reconsider if I saw clear evidence of a performance or usability
advantage on a variety of systems.