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

[Chris Evans]

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
Marrin:
http://mac-os-forge.2317878.n4.nabble.com/Arena-is-crufty-td178154.html

Whether or not we use RenderArena for additional things, or even remove it,
seems independent of the clean-up covered by
https://bugs.webkit.org/show_bug.cgi?id=101087.

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


Cheers
Chris


>
> _______________________________________________
> 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>