I think my earlier testing was faulty. Now when I test case 2, I get something comparable with and without the patch. If there is a regression, it's below the noise. Running it through a profiler shows a negligible amount of time in the new code.
I had tried running it through Dromaeo first, but any performance impact (if there is any) was well below the variance. I can take a stab at running Peacekeeper and Acid3 tomorrow, but I don't have high hopes of getting useful information out of them. My hope with the microbenchmarks was to show worst-case behavior. I expect that in practice this will have no performance impact in the real world. That's a hard statement to prove though. I'll see if I can reduce the variance in my microbenchmarks. On Thu, Mar 1, 2012 at 5:27 PM, Maciej Stachowiak <m...@apple.com> wrote: > > 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