Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
427803003:
Oilpan: Build fix after r179102 (Closed)
|
Created:
6 years, 4 months ago by haraken Modified:
6 years, 4 months ago CC:
blink-reviews, blink-reviews-css, ed+blinkwatch_opera.com, dglazkov+blink, apavlov+blink_chromium.org, darktears, rune+blink, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionOilpan: Build fix after r179102
It looks like r179102 increased sizeof(CSSValue) only in Windows builds with Oilpan enabled.
I don't know why. At the moment, this CL fixes the size expectation and fixed the compile error.
TBR=yukishiino@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179158
Patch Set 1 #
Total comments: 1
Messages
Total messages: 8 (0 generated)
Message was sent while issue was closed.
Committed patchset #1 manually as r179158 (presubmit successful).
Message was sent while issue was closed.
shiino-san: Would you take a look at why r179102 increased sizeof(CSSValue) only in Windows builds with Oilpan enabled? The compile error log is here: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20...
Message was sent while issue was closed.
https://codereview.chromium.org/427803003/diff/1/Source/core/css/CSSValue.cpp File Source/core/css/CSSValue.cpp (right): https://codereview.chromium.org/427803003/diff/1/Source/core/css/CSSValue.cpp... Source/core/css/CSSValue.cpp:61: struct SameSizeAsCSSValue : public RefCountedWillBeGarbageCollectedFinalized<SameSizeAsCSSValue>, public ScriptWrappableBase { We should add ScriptWrappableBase only if ENABLE(OILPAN) && OS(WIN) in order to check size increase in other conditions.
Message was sent while issue was closed.
On 2014/07/29 23:00:01, tkent wrote: > https://codereview.chromium.org/427803003/diff/1/Source/core/css/CSSValue.cpp > File Source/core/css/CSSValue.cpp (right): > > https://codereview.chromium.org/427803003/diff/1/Source/core/css/CSSValue.cpp... > Source/core/css/CSSValue.cpp:61: struct SameSizeAsCSSValue : public > RefCountedWillBeGarbageCollectedFinalized<SameSizeAsCSSValue>, public > ScriptWrappableBase { > We should add ScriptWrappableBase only if ENABLE(OILPAN) && OS(WIN) in order to > check size increase in other conditions. Updated a fix here: https://codereview.chromium.org/423293002/
Message was sent while issue was closed.
On 2014/07/29 16:29:39, haraken wrote: > shiino-san: Would you take a look at why r179102 increased sizeof(CSSValue) only > in Windows builds with Oilpan enabled? > > The compile error log is here: > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20... I found the cause: VC++ (including the latest VC++ 2013) doesn't support EBCO (Empty Base Class Optimization). For details, see http://social.msdn.microsoft.com/forums/vstudio/en-US/504c6598-6076-4acf-96b6... This article best describes the issue, and also introduces an workaround. I think we have at least two options here. 1) Oilpan reduces the size of CSSValue from 8 to 4 bytes because we no longer need a reference counter in CSSValue, we could leave this issue as-is until VC++ supports EBCO. The size of CSSValue is 8 bytes if it derives ScriptWrappableBase regardless of Oilpan. It's enough small, isn't it? 2) Does a workaround as introduced in the above article. class ScriptWrappableGarbageCollected : public GarbageCollected<ScriptWrappableGarbageCollected>, public ScriptWrappable {}; class CSSValue : public ScriptWrappableGarbageCollected { ... }; Then, CSSValue has only one direct base class which is empty, so it won't increase the size of CSSValue.
Message was sent while issue was closed.
On 2014/07/30 08:16:28, Yuki wrote: > On 2014/07/29 16:29:39, haraken wrote: > > shiino-san: Would you take a look at why r179102 increased sizeof(CSSValue) > only > > in Windows builds with Oilpan enabled? > > > > The compile error log is here: > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20... > > I found the cause: VC++ (including the latest VC++ 2013) doesn't support EBCO > (Empty Base Class Optimization). > For details, see > http://social.msdn.microsoft.com/forums/vstudio/en-US/504c6598-6076-4acf-96b6... > This article best describes the issue, and also introduces an workaround. > > I think we have at least two options here. > > 1) Oilpan reduces the size of CSSValue from 8 to 4 bytes because we no longer > need a reference counter in CSSValue, we could leave this issue as-is until VC++ > supports EBCO. The size of CSSValue is 8 bytes if it derives > ScriptWrappableBase regardless of Oilpan. It's enough small, isn't it? > > 2) Does a workaround as introduced in the above article. > class ScriptWrappableGarbageCollected : public > GarbageCollected<ScriptWrappableGarbageCollected>, public ScriptWrappable {}; > class CSSValue : public ScriptWrappableGarbageCollected { ... }; > Then, CSSValue has only one direct base class which is empty, so it won't > increase the size of CSSValue. Although not pretty, I would vote for solution (2). It would eliminate #if-defs since it works on all compilers without overhead. Just make sure to have a good explanation preceding the dummy-class definition.
Message was sent while issue was closed.
On 2014/07/30 08:44:45, zerny-chromium wrote: > On 2014/07/30 08:16:28, Yuki wrote: > > On 2014/07/29 16:29:39, haraken wrote: > > > shiino-san: Would you take a look at why r179102 increased sizeof(CSSValue) > > only > > > in Windows builds with Oilpan enabled? > > > > > > The compile error log is here: > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20... > > > > I found the cause: VC++ (including the latest VC++ 2013) doesn't support EBCO > > (Empty Base Class Optimization). > > For details, see > > > http://social.msdn.microsoft.com/forums/vstudio/en-US/504c6598-6076-4acf-96b6... > > This article best describes the issue, and also introduces an workaround. > > > > I think we have at least two options here. > > > > 1) Oilpan reduces the size of CSSValue from 8 to 4 bytes because we no longer > > need a reference counter in CSSValue, we could leave this issue as-is until > VC++ > > supports EBCO. The size of CSSValue is 8 bytes if it derives > > ScriptWrappableBase regardless of Oilpan. It's enough small, isn't it? > > > > 2) Does a workaround as introduced in the above article. > > class ScriptWrappableGarbageCollected : public > > GarbageCollected<ScriptWrappableGarbageCollected>, public ScriptWrappable {}; > > class CSSValue : public ScriptWrappableGarbageCollected { ... }; > > Then, CSSValue has only one direct base class which is empty, so it won't > > increase the size of CSSValue. > > Although not pretty, I would vote for solution (2). It would eliminate #if-defs > since it works on all compilers without overhead. Just make sure to have a good > explanation preceding the dummy-class definition. Sent out http://crrev.com/423383003 with solution 2. |
||||||||||||||||||||||||||||||||||||||||||||||||||
