[Oilpan] Prepare full definition of classes before using Member

Prepare full definition of classes before using Member<T>

What we want to do is to instantiate some templates of
<T> with T's full definition, in Member<T>::get() and
Member<T>::checkPointer().
To guarantee that template instantiations come after
T's definition, we put static_assert() in Member<T>.
See the change in platform/heap/Handle.h.

To achieve the condition, this CL does either of
1) move implementation of methods using Member<T> from .h to .cpp
2) include necessary .h
(1) can change inline definitions to outline definitions,
and it may regress performance a bit.

BUG=556947
Committed: https://crrev.com/6486e0c8e845fc96ad9579b7c6b67e4d474f8bb2
Cr-Commit-Position: refs/heads/master@{#360570}

[peria, 2015-11-18 12:10:31]

Description was changed from

==========
Move implementation to .cpp files


BUG=556947
==========

to

==========
**This CL is not for commit**
Prepare full definition of classes before using Member<>

What we want to do is to instantiate some templates of
<A> with A's full definition, in Member<>::get() and
Member<>::checkPointer().
To guarantee that template instantiations come after
A's definition, we put static_assert() in Member<>.

To achieve the condition, this CL does either of
1) move inside of methods using Member<> from .h to .cpp,
2) include A.h
(1) can change inline definition to outline definition,
and may regress performance a bit.


BUG=556947
==========

[peria, 2015-11-18 12:21:28]

peria@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[peria, 2015-11-18 12:21:29]

Hi team,

To avoid the inconsistent behaviors with
IsFullyDefined<T>::value(*), I made this CL as a reference.
This CL will be splitted into some small CLs to commit.

Could you take a glance and check where you have concerns?


Thank you in advance.


(*)
https://codereview.chromium.org/1411603007/diff/450001/third_party/WebKit/Sou...

[peria, 2015-11-18 12:22:21]

Description was changed from

==========
**This CL is not for commit**
Prepare full definition of classes before using Member<>

What we want to do is to instantiate some templates of
<A> with A's full definition, in Member<>::get() and
Member<>::checkPointer().
To guarantee that template instantiations come after
A's definition, we put static_assert() in Member<>.

To achieve the condition, this CL does either of
1) move inside of methods using Member<> from .h to .cpp,
2) include A.h
(1) can change inline definition to outline definition,
and may regress performance a bit.


BUG=556947
==========

to

==========
**This CL is not for commit**
Prepare full definition of classes before using Member<>

What we want to do is to instantiate some templates of
<A> with A's full definition, in Member<>::get() and
Member<>::checkPointer().
To guarantee that template instantiations come after
A's definition, we put static_assert() in Member<>.

To achieve the condition, this CL does either of
1) move inside of methods using Member<> from .h to .cpp,
2) include A.h
(1) can change inline definition to outline definition,
and may regress performance a bit.


BUG=556947
NOCOMMIT=true
==========

[haraken, 2015-11-19 01:37:25]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-11-19 01:37:26]

If this compiles, I'm fine with landing this CL.

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSDefaultStyleSheets.h (right):

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSDefaultStyleSheets.h:35: class
StyleSheetContents;

You can remove these forward declarations.

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLInputElement.h:41: class
HTMLImageLoader;

Ditto.

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/forms/InputTypeView.h (right):

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/forms/InputTypeView.h:128:
InputTypeView(HTMLInputElement&);

Add explicit.

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/storage/StorageEvent.cpp (right):

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/storage/StorageEvent.cpp:85: return
m_storageArea.get();

BTW, get() is not needed to get a raw pointer from a Member. We can remove these
get()s once we remove all WillBe types. (We cannot simply remove these get()s as
long as we have OwnPtrWillBeMember because OwnPtr needs a get() to get a raw
pointer.)

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Handle.h:776:
static_assert(IsFullyDefined<T>::value, "T is not fully defined.");

Would you replace other static_assert(sizeof(T)) in the code base with
static_assert(IsFullyDefined<T>::value) in a follow-up CL?

[peria, 2015-11-19 03:57:51]

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/InterpolationEnvironment.h
(right):

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/InterpolationEnvironment.h:9: #include
"core/svg/properties/SVGProperty.h"
These include are required.

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLInputElement.h:41: class
HTMLImageLoader;
On 2015/11/19 01:37:26, haraken wrote:
> 
> Ditto.

Acknowledged.

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/forms/InputTypeView.h (right):

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/forms/InputTypeView.h:128:
InputTypeView(HTMLInputElement&);
On 2015/11/19 01:37:26, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/storage/StorageEvent.cpp (right):

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/storage/StorageEvent.cpp:85: return
m_storageArea.get();
On 2015/11/19 01:37:26, haraken wrote:
> 
> BTW, get() is not needed to get a raw pointer from a Member. We can remove
these
> get()s once we remove all WillBe types. (We cannot simply remove these get()s
as
> long as we have OwnPtrWillBeMember because OwnPtr needs a get() to get a raw
> pointer.)

Thank you for pointing it out, and I removed some get() in this CL.
(This m_storageArea is not a target, though)
But I feel a complete clean-up is not required in this CL.

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1455943002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/Handle.h:776:
static_assert(IsFullyDefined<T>::value, "T is not fully defined.");
On 2015/11/19 01:37:26, haraken wrote:
> 
> Would you replace other static_assert(sizeof(T)) in the code base with
> static_assert(IsFullyDefined<T>::value) in a follow-up CL?

Acknowledged.

[haraken, 2015-11-19 04:09:23]

LGTM assuming the compile passes.

[peria, 2015-11-19 04:09:25]

The CQ bit was checked by peria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 04:09:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455943002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455943002/40001

[haraken, 2015-11-19 04:09:59]

Please update the CL title before landing.

[peria, 2015-11-19 04:18:03]

Description was changed from

==========
**This CL is not for commit**
Prepare full definition of classes before using Member<>

What we want to do is to instantiate some templates of
<A> with A's full definition, in Member<>::get() and
Member<>::checkPointer().
To guarantee that template instantiations come after
A's definition, we put static_assert() in Member<>.

To achieve the condition, this CL does either of
1) move inside of methods using Member<> from .h to .cpp,
2) include A.h
(1) can change inline definition to outline definition,
and may regress performance a bit.


BUG=556947
NOCOMMIT=true
==========

to

==========
Prepare full definition of classes before using Member<T>

What we want to do is to instantiate some templates of
<T> with T's full definition, in Member<T>::get() and
Member<T>::checkPointer().
To guarantee that template instantiations come after
T's definition, we put static_assert() in Member<T>.
See the change in platform/heap/Handle.h.

To achieve the condition, this CL does either of
1) move implementation of methods using Member<T> from .h to .cpp
2) include necessary .h
(1) can change inline definitions to outline definitions,
and it may regress performance a bit.


BUG=556947
==========

[commit-bot: I haz the power, 2015-11-19 04:50:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 04:50:22]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[peria, 2015-11-19 05:27:41]

The CQ bit was checked by peria@chromium.org

[peria, 2015-11-19 05:27:42]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1455943002/#ps60001
(title: "Fix compile error")

[commit-bot: I haz the power, 2015-11-19 05:28:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455943002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455943002/60001

[commit-bot: I haz the power, 2015-11-19 06:50:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 06:50:27]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[peria, 2015-11-19 08:40:13]

The CQ bit was checked by peria@chromium.org

[peria, 2015-11-19 08:40:14]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1455943002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-11-19 08:40:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455943002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455943002/100001

[commit-bot: I haz the power, 2015-11-19 09:41:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 09:41:46]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[peria, 2015-11-19 10:43:29]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-11-19 10:43:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455943002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455943002/100001

[Hitoshi Yoshida, 2015-11-19 11:50:49]

The CQ bit was checked by peria@google.com

[Hitoshi Yoshida, 2015-11-19 11:50:50]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1455943002/#ps120001
(title: "CORE_EXPORT")

[Hitoshi Yoshida, 2015-11-19 11:50:52]

The CQ bit was unchecked by peria@google.com

[peria, 2015-11-19 11:50:59]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-11-19 11:51:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455943002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455943002/120001

[commit-bot: I haz the power, 2015-11-19 13:20:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-19 13:21:30]

Patchset 7 (id:??) landed as
https://crrev.com/6486e0c8e845fc96ad9579b7c6b67e4d474f8bb2
Cr-Commit-Position: refs/heads/master@{#360570}

[sof, 2015-11-20 10:52:23]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-11-20 10:52:24]

Will this really work & is it worth it overall? MSVC has a more eager
instantiation of templates, which means that Member<> is instantiated more
readily.

Which is why Windows bots started failing after this landed,


http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20...

Once you start following up on those breakages in e.g., TreeScope.h, you end up
having to #include files that create mutual recursion between headers. Which
will not compile.

[peria, 2015-11-21 01:18:43]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1461193003/ by peria@chromium.org.

The reason for reverting is: 1) This CL regressed performance for 10% in some
tests.
2) This CL happens compile error on win_blink_oilpan_dbg.


BUG=559311.

[sof, 2015-11-22 08:33:30]

On 2015/11/21 01:18:43, peria wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/1461193003/ by mailto:peria@chromium.org.
> 
> The reason for reverting is: 1) This CL regressed performance for 10% in some
> tests.
> 2) This CL happens compile error on win_blink_oilpan_dbg.
> 
> 
> BUG=559311.

This CL added more #includes to key Blink header files (Document.h, Element.h).

Just as a data point, I see a 7 min reduction in compile times with it reverted.
(Tested by trivially modifying platform/heap/Handle.h, which triggers a
recompile of most of Blink.)

[haraken, 2015-11-22 14:19:10]

On 2015/11/22 08:33:30, sof wrote:
> On 2015/11/21 01:18:43, peria wrote:
> > A revert of this CL (patchset #7 id:120001) has been created in
> > https://codereview.chromium.org/1461193003/ by mailto:peria@chromium.org.
> > 
> > The reason for reverting is: 1) This CL regressed performance for 10% in
some
> > tests.
> > 2) This CL happens compile error on win_blink_oilpan_dbg.
> > 
> > 
> > BUG=559311.
> 
> This CL added more #includes to key Blink header files (Document.h,
Element.h).
> 
> Just as a data point, I see a 7 min reduction in compile times with it
reverted.
> (Tested by trivially modifying platform/heap/Handle.h, which triggers a
> recompile of most of Blink.)

Agreed. I realized that this is not an acceptable approach.