Bug 74154 - [chromium] Use HashMap<..., OwnPtr<Tile>> for compositor tilemap
Summary: [chromium] Use HashMap<..., OwnPtr<Tile>> for compositor tilemap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on: 74159 78071
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-08 18:27 PST by Adrienne Walker
Modified: 2012-02-14 13:01 PST (History)
11 users (show)

See Also:


Attachments
Patch (8.94 KB, patch)
2011-12-08 19:11 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (7.62 KB, patch)
2011-12-08 20:26 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Rebased to ToT (9.30 KB, patch)
2012-02-07 18:31 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Rebased to ToT (7.60 KB, patch)
2012-02-10 12:35 PST, Adrienne Walker
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-12-08 18:27:35 PST
Use HashMap<..., OwnPtr<> > for Chromium compositor tilemap
Comment 1 Adrienne Walker 2011-12-08 19:11:59 PST
Created attachment 118514 [details]
Patch
Comment 2 James Robinson 2011-12-08 19:38:08 PST
Could we do the OwnPtr.h change separately? I'd like that bit to get a broader look from people who probably don't care much about chromium compositor internals.
Comment 3 Adrienne Walker 2011-12-08 19:47:56 PST
(In reply to comment #2)
> Could we do the OwnPtr.h change separately? I'd like that bit to get a broader look from people who probably don't care much about chromium compositor internals.

Sure, no problem.
Comment 4 Adrienne Walker 2011-12-08 20:26:13 PST
Created attachment 118520 [details]
Patch
Comment 5 Adrienne Walker 2011-12-13 16:52:05 PST
Anybody interested in reviewing this patch?
Comment 6 James Robinson 2011-12-13 16:56:33 PST
Comment on attachment 118520 [details]
Patch

Nice! R=me
Comment 7 WebKit Review Bot 2011-12-13 17:52:59 PST
Comment on attachment 118520 [details]
Patch

Clearing flags on attachment: 118520

Committed r102726: <http://trac.webkit.org/changeset/102726>
Comment 8 WebKit Review Bot 2011-12-13 17:53:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Ryosuke Niwa 2011-12-13 19:54:50 PST
It appears that this patch caused a Chromium Mac build failure: http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Tests%29/builds/10806
Comment 10 James Robinson 2011-12-13 20:21:22 PST
Error run through c++filt:


Undefined symbols:
  "__ZN3WTF6OwnPtrIN7WebCore17CCLayerTilingData4TileEEC1ERKS4_", referenced from:
      __ZN3WTF9HashTableISt4pairIiiES1_IS2_NS_6OwnPtrIN7WebCore17CCLayerTilingData4TileEEEENS_18PairFirstExtractorIS8_EENS_8PairHashIiiEENS_14PairHashTraitsINS5_16TileMapKeyTraitsENS_10HashTraitsIS7_EEEESE_E6rehashEi in libwebcore_platform.a(CCLayerTilingData.o)
      __ZN3WTF9HashTableISt4pairIiiES1_IS2_NS_6OwnPtrIN7WebCore17CCLayerTilingData4TileEEEENS_18PairFirstExtractorIS8_EENS_8PairHashIiiEENS_14PairHashTraitsINS5_16TileMapKeyTraitsENS_10HashTraitsIS7_EEEESE_E3addINS_17HashMapTranslatorISH_SC_EES2_NS_10PassOwnPtrIS6_EEEES1_INS_17HashTableIteratorIS2_S8_SA_SC_SH_SE_EEbERKT0_RKT1_ in libwebcore_platform.a(CCLayerTilingData.o)
ld: symbol(s) not found
clang: error: linker command failed with exit code 1 (use -v to see invocation)Undefined symbols:
  "WTF::OwnPtr<WebCore::CCLayerTilingData::Tile>::OwnPtr(WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> const&)", referenced from:
      WTF::HashTable<std::pair<int, int>, std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >, WTF::PairFirstExtractor<std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WTF::PairHash<int, int>, WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WebCore::CCLayerTilingData::TileMapKeyTraits>::rehash(int) in libwebcore_platform.a(CCLayerTilingData.o)
      std::pair<WTF::HashTableIterator<std::pair<int, int>, std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >, WTF::PairFirstExtractor<std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WTF::PairHash<int, int>, WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WebCore::CCLayerTilingData::TileMapKeyTraits>, bool> WTF::HashTable<std::pair<int, int>, std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >, WTF::PairFirstExtractor<std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WTF::PairHash<int, int>, WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WebCore::CCLayerTilingData::TileMapKeyTraits>::add<WTF::HashMapTranslator<WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WTF::PairHash<int, int> >, std::pair<int, int>, WTF::PassOwnPtr<WebCore::CCLayerTilingData::Tile> >(std::pair<int, int> const&, WTF::PassOwnPtr<WebCore::CCLayerTilingData::Tile> const&) in libwebcore_platform.a(CCLayerTilingData.o)
ld: symbol(s) not found


I'm not sure what that means.
Comment 11 Ryosuke Niwa 2011-12-13 20:53:33 PST
Undefined symbols:
  "__ZN3WTF6OwnPtrIN7WebCore17CCLayerTilingData4TileEEC1ERKS4_", referenced from:
      __ZNSt4pairIS_IiiEN3WTF6OwnPtrIN7WebCore17CCLayerTilingData4TileEEEEC2ERKS7_ in libwebcore_platform.a(CCLayerTilingData.o)
ld: symbol(s) not found
clang: error: linker command failed with exit code 1 (use -v to see invocation)

and

rniwa-macpro:webkit3 rniwa$ c++filt __ZN3WTF6OwnPtrIN7WebCore17CCLayerTilingData4TileEEC1ERKS4_
WTF::OwnPtr<WebCore::CCLayerTilingData::Tile>::OwnPtr(WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> const&)

rniwa-macpro:webkit3 rniwa$ c++filt __ZNSt4pairIS_IiiEN3WTF6OwnPtrIN7WebCore17CCLayerTilingData4TileEEEEC2ERKS7_
std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >::pair(std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > const&)

So... something is screwed up here. We shouldn't be calling the copy constructor of OwnPtr. I haven't been following HashMap's OwnPtr support enough to tell what we need to do here.
Comment 12 James Robinson 2011-12-13 21:15:49 PST
Going to roll this patch out for now, the fix does not appear obvious.
Comment 13 James Robinson 2011-12-13 21:20:03 PST
Reverted r102726 for reason:

Does not compile on clang

Committed r102738: <http://trac.webkit.org/changeset/102738>
Comment 14 Adrienne Walker 2011-12-14 10:26:11 PST
(In reply to comment #13)
> Reverted r102726 for reason:
> 
> Does not compile on clang
> 
> Committed r102738: <http://trac.webkit.org/changeset/102738>

rniwa: Thanks for rolling this out.  Sorry for the hassle!

thakis: This looks a lot like the issue in https://bugs.webkit.org/show_bug.cgi?id=73711#c24, where Clang takes umbrage with the declared-but-not-defined OwnPtr copy constructor that gets optimized out elsewhere.
Comment 15 Nico Weber 2011-12-14 10:31:34 PST
jamesr said he'd try removing that weird declared-for-an-ancient-gcc-but-not-defined-anywhere constructor. Has that happend?
Comment 16 James Robinson 2011-12-14 10:34:41 PST
No, and I don't have a particular timetable for trying that.
Comment 17 Nico Weber 2011-12-14 10:58:57 PST
enne: Then I'd try doing that first.
Comment 18 Adrienne Walker 2012-02-07 18:31:28 PST
Created attachment 125977 [details]
Rebased to ToT
Comment 19 Philippe Normand 2012-02-07 19:42:10 PST
Comment on attachment 125977 [details]
Rebased to ToT

Attachment 125977 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11453218
Comment 20 Adrienne Walker 2012-02-07 19:49:23 PST
(In reply to comment #19)
> (From update of attachment 125977 [details])
> Attachment 125977 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11453218

Whoops, didn't mean to include the patch from bug 78071 as well.
Comment 21 Early Warning System Bot 2012-02-07 20:46:12 PST
Comment on attachment 125977 [details]
Rebased to ToT

Attachment 125977 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11449243
Comment 22 Gyuyoung Kim 2012-02-07 21:06:59 PST
Comment on attachment 125977 [details]
Rebased to ToT

Attachment 125977 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11449249
Comment 23 Adrienne Walker 2012-02-10 12:35:00 PST
Created attachment 126555 [details]
Rebased to ToT
Comment 24 James Robinson 2012-02-10 18:30:54 PST
Comment on attachment 126555 [details]
Rebased to ToT

View in context: https://bugs.webkit.org/attachment.cgi?id=126555&action=review

R=me

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:50
> +    static PassOwnPtr<DrawableTile> create() { return adoptPtr(new DrawableTile()); }

nit: i normally omit the () from the c'tor call here. not a strong preference
Comment 25 Adrienne Walker 2012-02-14 13:01:28 PST
Committed in r107645.