I agree with Adam's remarks. The safety benefit seems great, but we should investigate ways to get it at less performance cost (ideally no measurable cost).
I'm also curious what impact this change has on less micro- but still DOM-oriented benchmarks, such as Dromaeo's DOM tests, Peacekeeper, or the Acid3 secret hidden perf test. I think all of those entail some DOM node destruction although perhaps they do not hit this pattern at all. Regards, Maciej On Mar 1, 2012, at 5:06 PM, Adam Barth wrote: > Do we understand what's causing the performance regression? For > example, there are other implementation approaches where we try to > transfer the last ref rather than churning it or where we could use a > free list rather than a vector. I just wonder if there's a way to get > the benefits with a lower cost. > > Adam > > > On Thu, Mar 1, 2012 at 4:50 PM, Ojan Vafai <o...@chromium.org> wrote: >> We have a lot of code (e.g. in ContainerNode.cpp or any of the editing code) >> that needs to RefPtr nodes to make sure they're not destroyed due to >> synchronous events like blur, mutation events, etc. For example, >> ContainerNode::removeChild needs to RefPtr the parent to make it's not >> destroyed during a sync event. >> >> Currently, we need to write careful code that knows whether any methods it's >> calling can fire sync events and RefPtrs all the appropriate nodes. I have a >> proposal to automate this in https://bugs.webkit.org/show_bug.cgi?id=80041. >> It certainly makes the code cleaner and safer, but it comes at a performance >> cost in some cases. >> >> Basically, any scope that needs to be careful of sync events adds a >> DOMRemovalScope at the top which keeps nodes from getting destroyed until >> the scope is destroyed (DOMRemovalScope adds them to a >> Vector<RefPtr<Node>>). In cases where we don't delete nodes, there's no >> performance impact. In cases where we delete nodes, this is always slower. >> >> I uploaded two performance tests to that bug: >> 1. Set innerHTML = "" on a node that has half a million children. This test >> goes from ~166ms to ~172ms on my machine. A 3.6% regression. >> 2. Destroy half a million nodes *during* a synchronous event (uses >> range.deleteContents). Goes from ~284ms to ~342ms. A 20% regression. >> >> So destroying Nodes during synchronous events fired during a DOM mutation is >> significantly slower. This case is rare though. I had to think hard to come >> up with a case where we would hit this. For the most part, nodes don't >> actually get destroyed due to JS until a garbage collection occurs, which is >> usually after the event has completed and the DOMRemovalScope has been >> destroyed. >> >> The advantage of this is that it gives a simple way to address a common >> class of crash and potentially security bugs. The disadvantage of course is >> the possible performance hit. >> >> What do you think? Is this worth trying? Are there better ideas? >> >> Ojan >> >> _______________________________________________ >> webkit-dev mailing list >> webkit-dev@lists.webkit.org >> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev >> > _______________________________________________ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
