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

[Ryosuke Niwa]

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:
>
>> 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=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 agree that Eric’s r+ seems reasonable after reading the comments on the
bug. I think the miscommunication comes from how people interpreted Sam’s
objection on the bug:

> I would rather not generalize this.  Our long term plan is to stop using
> the RenderArena, so changes to use it in more places is probably not a
> great idea.
>
> If you have specific other uses where this type of allocator would be
> useful that would be interesting, but we should have those use cases
> established before moving forward.


One might consider this comment as simply asking to define the use case, in
which case Eric’s following comment adequately addressed the concern:

> cevans would like to use it for allocating DOM Nodes.  He has a prototype
> patch.  I agree we should probably wait to see how that bears fruit before
> generalizing.


Others might take it as objecting to the change until we can establish that
the use case behind the change is overall benefit to the project, in which
case, nobody has provided enough information anywhere thus far.

Now, it is my understanding that many members of the WebKit community
strongly felt the need to get rid of RenderArena eventually for some time.
Thus I was surprised when I learned that Chris (and possibly other members
of the community) wants to keep RenderArena. I would have preferred that
the change of this nature to be discussed on webkit-dev before the patch is
posted. e.g. Geoff was able to give us quite a few reasons we might not
want to proceed with the proposed generalization of RenderArena.

With respect to the specific patch being posted to the bug, while I agree
that the patch doesn’t deploy RenderArena in more places, the fact
RenderArena implementation isn’t specific to render objects doesn’t
necessarily mean we need to move it to WTF. If a class is only used in some
component of WebKit and we don’t intend to use it elsewhere, then it’s not
necessarily a bad idea to confine the visibility of the class in that
component so as not to pollute the WTF namespace.

Also, was there a reason why this change had to be made on trunk before
gathering more data? It seems like Chris already generalized RenderArena
locally and got some pref. data. Why can't we proceed with gathering more
data to reach a consensus as the community before landing patches given
that many of members of the community feel strongly that we should get rid
of RenderArena?

The way I see it, adding a build flag for this sort of things should be our
last report when we can’t reach a consensus. Adding a new build flag always
incurs a maintenance cost and forces us to "know more". Adding a build flag
to change memory allocations is particularly because that will
fundamentally change the nature of WebKit’s performance and security model.
e.g. how do we decide whether to take a performance optimization when the
optimization favors one memory management model over the other?

- R. Niwa
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20121114/9073af77/attachment.html>