Bug 74154 – [chromium] Use HashMap<..., OwnPtr<Tile>> for compositor tilemap

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

Use HashMap<..., OwnPtr<> > for Chromium compositor tilemap

[Adrienne Walker]

Created attachment 118514 [details]
Patch

[James Robinson]

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.

[Adrienne Walker]

(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.

[Adrienne Walker]

Created attachment 118520 [details]
Patch

[Adrienne Walker]

Anybody interested in reviewing this patch?

[James Robinson]

Comment on attachment 118520 [details]
Patch

Nice! R=me

[WebKit Review Bot]

Comment on attachment 118520 [details]
Patch

Clearing flags on attachment: 118520

Committed r102726: <http://trac.webkit.org/changeset/102726>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

It appears that this patch caused a Chromium Mac build failure: http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Tests%29/builds/10806

[James Robinson]

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.

[Ryosuke Niwa]

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.

[James Robinson]

Going to roll this patch out for now, the fix does not appear obvious.

[James Robinson]

Reverted r102726 for reason:

Does not compile on clang

Committed r102738: <http://trac.webkit.org/changeset/102738>

[Adrienne Walker]

(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.

[Nico Weber]

jamesr said he'd try removing that weird declared-for-an-ancient-gcc-but-not-defined-anywhere constructor. Has that happend?

[James Robinson]

No, and I don't have a particular timetable for trying that.

[Nico Weber]

enne: Then I'd try doing that first.

[Adrienne Walker]

Created attachment 125977 [details]
Rebased to ToT

[Philippe Normand]

Comment on attachment 125977 [details]
Rebased to ToT

Attachment 125977 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11453218

[Adrienne Walker]

(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.

[Early Warning System Bot]

Comment on attachment 125977 [details]
Rebased to ToT

Attachment 125977 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11449243

[Gyuyoung Kim]

Comment on attachment 125977 [details]
Rebased to ToT

Attachment 125977 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11449249

[Adrienne Walker]

Created attachment 126555 [details]
Rebased to ToT

[James Robinson]

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

[Adrienne Walker]

Committed in r107645.