[webkit-dev] scoping Node destruction during DOM modifications

[Ojan Vafai]

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 <mjs at 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 <ojan at 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 at lists.webkit.org
> >> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
> >>
> > _______________________________________________
> > webkit-dev mailing list
> > webkit-dev at lists.webkit.org
> > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120301/1841a08f/attachment.html>