On Wed, Nov 14, 2012 at 3:27 PM, Ojan Vafai <o...@chromium.org> wrote:
> On Wed, Nov 14, 2012 at 2:48 PM, Geoffrey Garen <gga...@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@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@lists.webkit.org > http://lists.webkit.org/mailman/listinfo/webkit-dev > >
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
