[webkit-dev] RenderArena: Teaching an old dog new tricks

Chris Evans cevans at chromium.org
Wed Nov 14 15:36:25 PST 2012

On Wed, Nov 14, 2012 at 3:27 PM, Ojan Vafai <ojan at chromium.org> wrote:

> On Wed, Nov 14, 2012 at 2:48 PM, Geoffrey Garen <ggaren at apple.com> wrote:
>> Hi Eric.
>> Here are some problems in RenderArena that I know of:
>> - Grows memory without bound
>> - Duplicates the functionality of FastMalloc
>> - Makes allocation error-prone (allocation in the wrong arena is
>> sometimes a leak, sometimes a use-after-free, and sometimes a heizenbug of
>> the two)
>> - Makes allocation verbose (you have to thread an arena pointer
>> everywhere)
>> - Makes object lifetime complicated (all objects are implicitly tied to a
>> single owner they may outlive)
>> - Uses C-style macros and manual initialization and destruction, instead
>> of modern WebKit C++ style
>> You didn't mention any of these problems in your email, so I'll assume
>> you weren't aware of them.
>> Considering these problems now, please don't use RenderArena in more
>> places.
>> > Slab-allocators (i.e. RenderArena) hand out memory all from a single
>> > region, guaranteeing (among other things) that free'd objects can only
>> > be ever overwritten by other objects from the same pool.  This makes
>> > it much harder, for example to find a Use-After-Free of a RenderBlock
>> > and then fill that RenderBlock's memory (and vtable pointer) with
>> > arbitrary memory (like the contents of a javascript array).
>> > http://en.wikipedia.org/wiki/Slab_allocation
>> This is magical thinking. RenderArena is no different from FastMalloc.
>> (1) RenderArena recycles by object size, just like FastMalloc.
>> (2) FastMalloc is a slab allocator, just like RenderArena.
>> (3) RenderArena grows by calling FastMalloc.
>> Isolating object types from each other -- and specifically isolating
>> objects of arbitrary size and contents like arrays -- is an interesting
>> idea. RenderArena is neither necessary nor sufficient for implementing this
>> feature.
>> The only reason RenderArena seems isolated from other object types is
>> social, not technical: we actively discourage using RenderArena, so few
>> object types currently use it.
>> > Since RenderArena is generic, the current plan to move it to WTF (as
>> > by Chris Marrin suggested back in
>> > http://www.mail-archive.com/webkit-dev@lists.webkit.org/msg12672.html),
>> > clean up the code further, and investigate wider deployment (like to
>> > the DOM tree) for the security benefit and possible perf win.
>> > https://bugs.webkit.org/show_bug.cgi?id=101087
>> Having dealt with the specific technical question of RenderArena, I'd
>> like to briefly discuss the meta-level of how the WebKit project works.
>> Sam Weinig and I both provided review feedback saying that using
>> RenderArena more was a bad idea (https://bugs.webkit.org/show_bug.cgi?id<https://bugs.webkit.org/show_bug.cgi?id=101087#c9>
> *This seems completely unfair *
> *
>> *
> *Geoff*
> *
>> *
> *_______________________________________________*
> *webkit-dev mailing list*
> *webkit-dev at lists.webkit.org*
> *http://lists.webkit.org/mailman/listinfo/webkit-dev*
> =101087#c9 <https://bugs.webkit.org/show_bug.cgi?id=101087#c9>,
>> https://bugs.webkit.org/show_bug.cgi?id=101087#c18). Nonetheless, you
>> r+ed a patch to move in that direction, and you describe it here as the
>> "current plan" for WebKit.
>> I'm a little disappointed that, as individual contributors, Chris Neklar
>> and Chris Evans didn't realize or understand the problems listed above, and
>> didn't tackle them. However, the mistake is understandable: Chris and Chris
>> are new to WebKit. The WebKit project has a mechanism for resolving
>> mistakes like this: patch review.
>> Your job as a reviewer is to understand the zeitgeist of the project, to
>> use good judgement, and to r- patches that make mistakes like this. A bad
>> patch is only a small nuisance. But the small nuisance turns into a major
>> problem when you, as a reviewer, take a bad patch, mark it r+, and declare
>> it the current direction of the project, despite the objections of two
>> other reviewers who are senior members of the project.
> As someone outside all these discussions, this seems like a completely
> unfair characterization of what happened. Sam voiced an objection, then
> there was a bunch of discussion in which Chris made an argument that Eric
> found compelling. Many days passed, then Eric r+'ed. Unless there was other
> discussion not on the bug that I missed, your objection came after Eric's
> r+. I don't see the process problem here.

I think there's still a massive misunderstanding here. The bug in question,
https://bugs.webkit.org/show_bug.cgi?id=101087, is simply renaming / moving
a generic class into a more generic location. There's nothing render
specific about RenderArena.

Moving RenderArena to a more generic location, based on the observation
that it isn't render specific, has been floated as early as 2010 by Chris

Whether or not we use RenderArena for additional things, or even remove it,
seems independent of the clean-up covered by

It's very exciting that the concept of DOM slabbing is generating a good
discussion :)


> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo/webkit-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20121114/37761551/attachment.html>

More information about the webkit-dev mailing list