Bug 122895

Summary: Take RenderObjects out of the arena.
Product: WebKit Reporter: Andreas Kling <kling>
Component: Layout and RenderingAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, ggaren, koivisto
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Andreas Kling 2013-10-16 08:46:38 PDT
We should stop arena-allocating RenderObjects. This will allow us to strictify the code and improve the memory management.
Comment 1 Andreas Kling 2013-10-16 08:48:52 PDT
Created attachment 214364 [details]
Patch
Comment 2 Antti Koivisto 2013-10-16 09:03:42 PDT
Comment on attachment 214364 [details]
Patch

r=me
Comment 3 Geoffrey Garen 2013-10-16 09:09:14 PDT
I think this will break two things:

(1) The "relevant repainted objects" heuristic. It's based on the number of objects in the arena.

(2) 1% regression on PLT. (Not sure if this translates into a regression on PLT3.)
Comment 4 Antti Koivisto 2013-10-16 09:46:13 PDT
Vast majority of arena allocated objects are line boxes. This patch is not the same as removing the arena.
Comment 5 Geoffrey Garen 2013-10-16 10:51:34 PDT
(In reply to comment #4)
> Vast majority of arena allocated objects are line boxes. This patch is not the same as removing the arena.

OK, sounds like my PLT concern was inaccurate.

However, I'm still worried about the "relevant repainted objects" heuristic. We use that one pages that don't have much text, so it may depend on observing render objects that are not line boxes.
Comment 6 Beth Dakin 2013-10-16 11:17:06 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Vast majority of arena allocated objects are line boxes. This patch is not the same as removing the arena.
> 
> OK, sounds like my PLT concern was inaccurate.
> 
> However, I'm still worried about the "relevant repainted objects" heuristic. We use that one pages that don't have much text, so it may depend on observing render objects that are not line boxes.

Relevant repainted objects do not actually rely on arena size. You are thinking of Page::renderTreeSize(), which is exposed to clients through WKPageGetRenderTreeSize() andWKBundlePageGetRenderTreeSize(). That might be broken.
Comment 7 Andreas Kling 2013-10-16 15:11:44 PDT
Comment on attachment 214364 [details]
Patch

After discussing this with Antti, I'm gonna go ahead and get this landed so we can see about the performance impact on bots.

We're going to need a replacement for the hueristic that currently depends on Page::renderTreeSize() no matter what, so we shouldn't let this block on that.
Comment 8 WebKit Commit Bot 2013-10-16 15:26:53 PDT
Comment on attachment 214364 [details]
Patch

Clearing flags on attachment: 214364

Committed r157535: <http://trac.webkit.org/changeset/157535>
Comment 9 WebKit Commit Bot 2013-10-16 15:26:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Geoffrey Garen 2013-10-16 16:03:05 PDT
> We're going to need a replacement for the hueristic that currently depends on Page::renderTreeSize() no matter what

Do we have a bug about that?
Comment 11 Antti Koivisto 2013-10-17 05:53:48 PDT
Some tests show some huge memory wins from this, 10MB or ~10% in html5-full-render:

https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mountainlion%22%2C%22Parser%2Fhtml5-full-render%3AMalloc%22%5D%5D

Wider performance tests seem unaffected, html5-full-render even looks like possible ~0.5% progression here:

https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mountainlion%22%2C%22Parser%2Fhtml5-full-render%3ATime%22%5D%5D&zoom=%5B1381725375901.368%2C1381985023560.4456%5D

A few micro-benchmarks regressed. Here is ~2.5% regression Dromaeo dom-modify subtest:

https://perf.webkit.org/#mode=charts&days=6&chartList=%5B%5B%22mac-mountainlion%22%2C%22Dromaeo%2Fdom-modify%3ARuns%22%5D%5D

These are tests that create and delete DOM nodes but don't render them (we create renderers synchronously but never use them, allocation time dominates). These will go away when we start creating render tree lazily: https://bugs.webkit.org/show_bug.cgi?id=120685