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