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.<div>

<br></div><div>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. </div>

<div><br></div><div>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.<br>

<br><div class="gmail_quote">On Thu, Mar 1, 2012 at 5:27 PM, Maciej Stachowiak <span dir="ltr"><<a href="mailto:mjs@apple.com">mjs@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
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).<br>
<br>
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.<br>


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