Bug 17425 - Eliminate DeprecatedPtrList
Summary: Eliminate DeprecatedPtrList
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
: 26194 (view as bug list)
Depends on: 54972
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-18 09:52 PST by Darin Adler
Modified: 2011-02-27 11:12 PST (History)
8 users (show)

See Also:


Attachments
work in progress (93.05 KB, patch)
2008-02-18 10:17 PST, Darin Adler
no flags Details | Formatted Diff | Diff
patch for part of this (25.72 KB, patch)
2008-12-14 08:37 PST, Darin Adler
no flags Details | Formatted Diff | Diff
another attempt (6.58 KB, patch)
2009-06-05 18:07 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (27.49 KB, patch)
2011-02-16 08:47 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (59.00 KB, patch)
2011-02-21 11:09 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (38.04 KB, patch)
2011-02-27 10:29 PST, Benjamin Poulain
kling: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2008-02-18 09:52:21 PST
I'm working on this one. It's a little tricky.
Comment 1 Darin Adler 2008-02-18 10:17:41 PST
Created attachment 19190 [details]
work in progress
Comment 2 Darin Adler 2008-12-13 17:28:04 PST
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.
Comment 3 Darin Adler 2008-12-13 20:05:32 PST
(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.
Comment 4 Darin Adler 2008-12-14 08:37:15 PST
Created attachment 26012 [details]
patch for part of this
Comment 5 Sam Weinig 2008-12-14 16:40:19 PST
Comment on attachment 26012 [details]
patch for part of this

> +++ ChangeLog	(working copy)
> @@ -1,3 +1,29 @@
> +2008-12-13  Cameron Zwarich  <zwarich@apple.com>
> +
> +        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.
Comment 6 Darin Adler 2008-12-14 17:49:19 PST
(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.
Comment 7 Darin Adler 2008-12-14 18:40:20 PST
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.
Comment 8 Darin Adler 2009-01-02 18:05:22 PST
http://trac.webkit.org/changeset/39566
Comment 9 Darin Adler 2009-01-03 11:17:39 PST
Oops, closed the wrong bug!
Comment 10 George Staikos 2009-04-06 20:38:16 PDT
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.
Comment 11 Darin Adler 2009-06-05 18:07:16 PDT
Created attachment 31023 [details]
another attempt
Comment 12 George Staikos 2009-06-10 05:44:25 PDT
(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.
Comment 13 Darin Adler 2009-06-12 14:02:45 PDT
(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.
Comment 14 Sam Weinig 2009-08-06 12:17:13 PDT
*** Bug 26194 has been marked as a duplicate of this bug. ***
Comment 15 Benjamin Poulain 2011-02-16 08:47:06 PST
Created attachment 82644 [details]
Patch
Comment 16 Benjamin Poulain 2011-02-16 08:48:26 PST
I will try that with Qt benchmarks, see if that causes any regressions.

George, do you had a specific test in mind?
Comment 17 Benjamin Poulain 2011-02-16 09:24:18 PST
(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.
Comment 18 Dave Hyatt 2011-02-16 10:28:40 PST
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.
Comment 19 Benjamin Poulain 2011-02-16 10:32:06 PST
(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.
Comment 20 Benjamin Poulain 2011-02-21 11:09:52 PST
Created attachment 83194 [details]
Patch
Comment 21 Benjamin Poulain 2011-02-21 11:21:43 PST
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.
Comment 22 Benjamin Poulain 2011-02-22 11:17:47 PST
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.
Comment 23 Benjamin Poulain 2011-02-27 10:29:54 PST
Created attachment 83978 [details]
Patch
Comment 24 WebKit Commit Bot 2011-02-27 10:34:21 PST
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
Comment 25 Benjamin Poulain 2011-02-27 11:12:32 PST
Committed r79825: <http://trac.webkit.org/changeset/79825>