Bug 8039 – Remove use of DeprecatedArray in favor of new Vector class

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Sam Weinig]

Most of the changes require only a change of name from DeprecatedArray<> to Vector<> in variable declarations and little else to retain the same functionality.  However, since the Vector class has some different/better qualities/features, other changes would be beneficial.

[Sam Weinig]

Created attachment 7382 [details]
patch

This removes quite a bit of the DeprecatedArray usage, but more remains.

[Sam Weinig]

Created attachment 7465 [details]
patch 2

This takes care of most of the DeprecatedArray uses.  Now all that remains is in the DeprecatedCString class (which I have not attempted because it is itself deprecated), and all the image relate code (which I is a little more complicated but will be in the next version).

[Eric Seidel (no email)]

Make sure to mark your patch for review :)

[Sam Weinig]

Created attachment 7488 [details]
patch 3

This is quite similar to the previous version with some minor adjustments and a changelog entry.  Image and friends are still using DeprecatedArray/DeprecatedByteArray.  Because of this, in CachedImage I had to copy from a Vector to a DeprecatedByteArray which I am worried is going to pretty bad performance wise.  However, when the Image classes change to a Vector based byte buffer, the problem should go away.

[Maciej Stachowiak]

We would need to ensure that this does not cause a performance regression.

[Eric Seidel (no email)]

Comment on attachment 7488 [details]
patch 3

I haven't forgotten you.  But we can't land this w/o performance testing, which I just haven't taken the time to do yet.

[Dave Hyatt]

Comment on attachment 7488 [details]
patch 3

I worked very hard to remove extra copies of the image data, so re-introducing one now is something we should avoid.

[Sam Weinig]

Created attachment 7964 [details]
patch the fourth

This version does not include the changes to the caching/loading classes that caused the expensive copying.

[Sam Weinig]

Created attachment 8078 [details]
patch for image and caching classes

This patch is changes just the Image and caching classes to use Vector instead of DeprecatedArray.  In order to not make any unecessary copies, some changes were made so that the data for an image is buffered directly into an internal buffer in the Image object.

[Dave Hyatt]

Comment on attachment 8078 [details]
patch for image and caching classes

This one looks fine to me coding-wise.  

Please fix the misspellings of the word "received" though.  "i" before "e" except after "c". :)

[Dave Hyatt]

Comment on attachment 7964 [details]
patch the fourth

Do Vectors ever shrink their capacities when you remove items or clear?  In some cases the old DeprecatedArrays are deliberately designed to never shrink, so I want to make sure Vector has the same behavior.

[Dave Hyatt]

Comment on attachment 7964 [details]
patch the fourth

If Vectors don't shrink capacity on clear, then this patch also changes behavior (which may or may not be ok).

For example in HTMLSelectElement:

-    m_listItems.resize(0);
+    m_listItems.clear();

The old code would actually re-shrink the array back down to 0.  The new code may retain a large capacity (which could in fact be better for performance in this case, but is a change in behavior).

These patches will definitely need to be performance-tested with all the benchmarks (our internal PLT, i-bench, and the 24fun JS test).

[Dave Hyatt]

Comment on attachment 7964 [details]
patch the fourth

The other thing that you should double-check is to make sure none of these DeprecatedArrays were being copied.  DeprecatedArrays = cheap to copy.  Vectors = expensive to copy.

r=me. (I'll + the other patch once the "recieved" typo is fixed.)

[Sam Weinig]

Created attachment 8195 [details]
updated image and caching patch

Fixed spelling mistakes and updated to use wtf instead of KXMLCore.

[Dave Hyatt]

Comment on attachment 8195 [details]
updated image and caching patch

r=me, can just land these two patches together now!

[Darin Adler]

I'm working on landing this.

[Darin Adler]

Committed revision 14816.