Bug 17425 – Eliminate DeprecatedPtrList

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Darin Adler]

I'm working on this one. It's a little tricky.

[Darin Adler]

Created attachment 19190 [details]
work in progress

[Darin Adler]

Remaining uses:

    1) CSSRuleList -- Moderate difficulty to change; should be a memory use improvement when we fix it. Area needs other kinds of cleanup too.

    2) Document image load event lists -- Should be trivial to replace these with ListHashSet.

    3) RenderBlock floating object list -- I don't know exactly what to replace that one with.

    4) Node chain in EventTargetNode::dispatchGenericEvent -- Should be trivial to replace this with Vector.

    5) m_pendingCallbacks in XMLTokenizer -- Should be replaced with Deque.

    6) unstyledSpans in ApplyStyleCommand::applyRelativeFontStyleChange -- Should be trivial to replace this with Vector.

[Darin Adler]

(In reply to comment #2)
>     2) Document image load event lists -- Should be trivial to replace these
> with ListHashSet.

Can't actually use ListHashSet for this, because we allow the same loader to be in the list twice.

[Darin Adler]

Created attachment 26012 [details]
patch for part of this

[Sam Weinig]

Comment on attachment 26012 [details]
patch for part of this

> +++ ChangeLog	(working copy)
> @@ -1,3 +1,29 @@
> [phone]  Cameron Zwarich  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 22562: REGRESSION (r37971): events not firing after going back in back/forward cache
> +        <https://bugs.webkit.org/show_bug.cgi?id=22562>
> +        <rdar://problem/6414593>
> +
> +        Restore the Frame's DOMWindow to its previous value when going back in
> +        the back/forward cache. The fact that it was not getting set before may
> +        have always caused some subtle bugs with the back/forward cache, but
> +        after r37971, it causes no events to fire after restoring a page.
> +
> +        Unfortunately, there is no way to test this with a layout test because
> +        it involves the back/forward cache.
> +
> +        * history/CachedPage.cpp:
> +        (WebCore::CachedPage::domWindow): Add.
> +        * history/CachedPage.h:
> +        * loader/FrameLoader.cpp:
> +        (WebCore::FrameLoader::open): Restore the DOMWindow of the Frame to its
> +        saved value when navigating in the back/forward cache.
> +        * page/Frame.cpp:
> +        (WebCore::Frame::setDOMWindow): Add.
> +        * page/Frame.h:

I don't think you meant to include this.


> +    // Set up a pointer to indicate whether to dispatch window events.
> +    // We don't dispatch load events to the window as a quirk, because
> +    // Mozilla used to never propagate load events to the window object.

Is this not true anymore?  Should we stop behaving this way?

r=me.

[Darin Adler]

(In reply to comment #5)
> > +    // Set up a pointer to indicate whether to dispatch window events.
> > +    // We don't dispatch load events to the window as a quirk, because
> > +    // Mozilla used to never propagate load events to the window object.
> 
> Is this not true anymore?  Should we stop behaving this way?

I'll try to correct the wording so this doesn't sound temporary. I don't think it's temporary -- and in fact someone more familiar with the situation could probably fix the comment further. When Hyatt originally wrote it a long time ago, we were less clear on things than we are now.

[Darin Adler]

Comment on attachment 26012 [details]
patch for part of this

Clearing review flag since I landed this as http://trac.webkit.org/changeset/39296 and took care of (2), (4), (5), and (6) above. Here's what remains:

    1) CSSRuleList::m_lstCSSRules. This can almost certainly be a Vector. But things are little tangled in the CSS classes at the moment and there is some other improvement that can be made too.

    3) RenderBlock::m_floatingObjects. Probably simple to change this to a ListHashSet as m_positionedObjects is. But it might be even more efficient if the objects didn't each have to be in a separate memory block.

The old patch attached to this bug includes has a cut at fixing both of these issues.

[Darin Adler]

http://trac.webkit.org/changeset/39566

[Darin Adler]

Oops, closed the wrong bug!

[George Staikos]

m_floatingObjects seems to be a really bad one actually.  There are some pages where this list gets to be enormous.  The problem is how to make this efficient without using more memory.  Just using a ListHashSet isn't enough since we have to index by the RenderBox.

[Darin Adler]

Created attachment 31023 [details]
another attempt

[George Staikos]

(In reply to comment #11)
> Created an attachment (id=31023) [review]
> another attempt
> 

The patch is only intended to remove the ptr list without improving performance, right?  It might make sense to leave this as-is until we can solve the performance issue because it's a nice big red flag about it.

[Darin Adler]

(In reply to comment #10)
> There are some pages where this list gets to be enormous.

Could you file a bug about the performance issue?

I don't think DeprecatedPtrList works as a big red flag. Nothing in the name says "bad for performance" to me.

I also think you're being a bit enigmatic here saying "some pages" and "enormous" without specifics. I'd like to see a bug report about the performance problem and some test cases that demonstrate the problem.

[Sam Weinig]

*** Bug 26194 has been marked as a duplicate of this bug. ***

[Benjamin Poulain]

Created attachment 82644 [details]
Patch

[Benjamin Poulain]

I will try that with Qt benchmarks, see if that causes any regressions.

George, do you had a specific test in mind?

[Benjamin Poulain]

(In reply to comment #16)
> I will try that with Qt benchmarks, see if that causes any regressions.

I tried the benchmarks and there is no difference in timing. But that is on regular websites, if someone have an idea of a better test case I would be happy to test.

[Dave Hyatt]

Comment on attachment 82644 [details]
Patch

You could have a Vector of OwnPtrs, and then you don't have to walk the list deleting everything.

I still think it's better if you're making this conversion to move to a ListHashSet instead though.  I'm not quite sure if ListHashSet has everything you need, since you have to construct the hash key by using the RenderObject* rather than the FloatingObject*, but then you'd be fixing the performance problem with large float lists at the same time.

It just feels strange to me to convert to another list data structure when we know that a list is not what we want to stick with long-term.

[Benjamin Poulain]

(In reply to comment #18)
> (From update of attachment 82644 [details])
> You could have a Vector of OwnPtrs, and then you don't have to walk the list deleting everything.
> 
> I still think it's better if you're making this conversion to move to a ListHashSet instead though.

I totally agree with you :)
I made that per George comments, hoping to get the discussion started on those pages with lots of float.

If you are ok with ListHashSet, I'll update the patch with that.

[Benjamin Poulain]

Created attachment 83194 [details]
Patch

[Benjamin Poulain]

ListHashTable interface is a lot like ListHashSet. I did not do the custom allocator because that would make the patch quite more complex and that does not have as much value for this use case as it had for the ListHashSet.

I can still add custom allocator in a followup patch without changing anything in RenderBlock.

I did not use RefPtr to store the FloatingObject because that would add quite some mess (with floatMap deleting objects its own way). If you think the memory management is not clear here, I can add RefPtr later and refactor RenderBlock for that. I think that could be in a follow up patch because this one is already quite big to review.

[Benjamin Poulain]

Comment on attachment 83194 [details]
Patch

Clearing the review flag, I split the diff in https://bugs.webkit.org/show_bug.cgi?id=54973 and https://bugs.webkit.org/show_bug.cgi?id=54972.

[Benjamin Poulain]

Created attachment 83978 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 83978 [details]
Patch

Rejecting attachment 83978 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
e.xcodeproj/project.pbxproj
patching file Source/WebCore/platform/DeprecatedPtrList.h
rm 'Source/WebCore/platform/DeprecatedPtrList.h'
patching file Source/WebCore/platform/DeprecatedPtrListImpl.cpp
rm 'Source/WebCore/platform/DeprecatedPtrListImpl.cpp'
patching file Source/WebCore/platform/DeprecatedPtrListImpl.h
rm 'Source/WebCore/platform/DeprecatedPtrListImpl.h'

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Andreas Kling', u'--fo..." exit_code: 1

Full output: http://queues.webkit.org/results/8070153

[Benjamin Poulain]

Committed r79825: <http://trac.webkit.org/changeset/79825>