Skip to content

Conversation

droberts195
Copy link
Contributor

Visual Studio doesn't implement the empty base class
optimisation by default in cases of multiple inheritance.
To preserve ABI backwards compatibility it only does this
when explicitly told.

The secret /d1reportClassLayoutChanges option will reveal
where we might benefit from using __declspec(empty_bases).

Relates #840

Visual Studio doesn't implement the empty base class
optimisation by default in cases of multiple inheritance.
To preserve ABI backwards compatibility it only does this
when explicitly told.

The secret `/d1reportClassLayoutChanges` option will reveal
where we might benefit from using `__declspec(empty_bases)`.

Relates elastic#840
@droberts195
Copy link
Contributor Author

droberts195 commented Nov 22, 2019

consoleText.txt from the Windows PR CI build contains the special output.

Search for "Future Default Layout" to see the opportunities.

For example one is:

Effective Layout: (Default)

class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > >    size(48):
    +---
 0  | +--- (base class ml::maths::CBasicStatistics::COrderStatisticsImpl<struct std::pair<double,class ml::maths::CDenseVector<double> >,class std::vector<struct std::pair<double,class ml::maths::CDenseVector<double> >,class std::allocator<struct std::pair<double,class ml::maths::CDenseVector<double> > > >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > >)
 0  | | ?$less@U?$pair@NV?$CDenseVector@N@maths@ml@@@std@@ m_Less
    | | <alignment member> (size=7)
 8  | | ?$vector@U?$pair@NV?$CDenseVector@N@maths@ml@@@std@@V?$allocator@U?$pair@NV?$CDenseVector@N@maths@ml@@@std@@@2@ m_Statistics
32  | | m_UnusedCount
    | +---
41  | +--- (base class boost::operators_impl::addable<class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > >,class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > >,class boost::operators_impl::operators_detail::empty_base<class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > > >,struct boost::operators_impl::operators_detail::false_t>)
41  | | +--- (base class boost::operators_impl::addable1<class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > >,class boost::operators_impl::operators_detail::empty_base<class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > > > >)
41  | | | +--- (base class boost::operators_impl::operators_detail::empty_base<class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > > >)
    | | | +---
    | | +---
    | +---
    | <alignment member> (size=7)
    +---

Future Default Layout: (Empty Base Class Optimization)

class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > >    size(40):
    +---
 0  | +--- (base class ml::maths::CBasicStatistics::COrderStatisticsImpl<struct std::pair<double,class ml::maths::CDenseVector<double> >,class std::vector<struct std::pair<double,class ml::maths::CDenseVector<double> >,class std::allocator<struct std::pair<double,class ml::maths::CDenseVector<double> > > >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > >)
 0  | | ?$less@U?$pair@NV?$CDenseVector@N@maths@ml@@@std@@ m_Less
    | | <alignment member> (size=7)
 8  | | ?$vector@U?$pair@NV?$CDenseVector@N@maths@ml@@@std@@V?$allocator@U?$pair@NV?$CThreadDataWriter.cc
CDenseVector@N@maths@ml@@@std@@@2@ m_Statistics
32  | | m_UnusedCount
    | +---
 0  | +--- (base class boost::operators_impl::addable<class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > >,class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > >,class boost::operators_impl::operators_detail::empty_base<class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > > >,struct boost::operators_impl::operators_detail::false_t>)
 0  | | +--- (base class boost::operators_impl::addable1<class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > >,class boost::operators_impl::operators_detail::empty_base<class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > > > >)
 0  | | | +--- (base class boost::operators_impl::operators_detail::empty_base<class ml::maths::CBasicStatistics::COrderStatisticsHeap<struct std::pair<double,class ml::maths::CDenseVector<double> >,struct std::less<struct std::pair<double,class ml::maths::CDenseVector<double> > > > >)
    | | | +---
    | | +---
    | +---
    +---

The second base class adds 1 byte to the object size, but then padding for alignment increases that to 8 bytes.

Many of the classes that would benefit from the optimisation appear to be in the standard library (which Microsoft will not change until the next ABI-breaking release of Visual C++) and Eigen (which we're not allowed to edit because it's under the MPL). Still, there may be a few opportunities for making our code more memory-efficient on Windows.

@droberts195
Copy link
Contributor Author

droberts195 commented Nov 22, 2019

This is a way to find our classes in the output:

bash-3.2$ grep '^class ml' consoleText.txt | sed 's/<.*size/ size/' | sort -u | tr '\t' ' '
class ml::maths::CBasicStatistics::COrderStatisticsHeap size(104):
class ml::maths::CBasicStatistics::COrderStatisticsHeap size(40):
class ml::maths::CBasicStatistics::COrderStatisticsHeap size(48):
class ml::maths::CBasicStatistics::COrderStatisticsHeap size(96):
class ml::maths::CBasicStatistics::COrderStatisticsStack size(32):
class ml::maths::CBasicStatistics::COrderStatisticsStack size(40):
class ml::maths::CBasicStatistics::COrderStatisticsStack size(48):
class ml::maths::CLogJointProbabilityOfLessLikelySamples size(48):
class ml::maths::CLogJointProbabilityOfLessLikelySamples size(56):
class ml::maths::CTimeSeriesDecomposition size(664):
class ml::maths::CTimeSeriesDecomposition size(672):
class ml::maths::`anonymous-namespace'::detail::CHashIterator size(16):
class ml::maths::`anonymous-namespace'::detail::CHashIterator size(8):
class ml::model::CHierarchicalResultsNormalizer size(200):

CHierarchicalResultsNormalizer only appears once in that list because although its layout changes its overall size doesn't - there's already padding and the padding just gets shuffled to different places.

So, the list of classes that can benefit from __declspec(empty_bases) is:

  1. ml::maths::CBasicStatistics::COrderStatisticsHeap
  2. ml::maths::CBasicStatistics::COrderStatisticsStack
  3. ml::maths::CLogJointProbabilityOfLessLikelySamples
  4. ml::maths::CTimeSeriesDecomposition
  5. ml::maths::`anonymous-namespace'::detail::CHashIterator

@edsavage
Copy link
Contributor

This is a way to find our classes in the output:

bash-3.2$ grep '^class ml' consoleText.txt | sed 's/<.*size/ size/' | sort -u | tr '\t' ' '
class ml::maths::CBasicStatistics::COrderStatisticsHeap size(104):
class ml::maths::CBasicStatistics::COrderStatisticsHeap size(40):
class ml::maths::CBasicStatistics::COrderStatisticsHeap size(48):
class ml::maths::CBasicStatistics::COrderStatisticsHeap size(96):
class ml::maths::CBasicStatistics::COrderStatisticsStack size(32):
class ml::maths::CBasicStatistics::COrderStatisticsStack size(40):
class ml::maths::CBasicStatistics::COrderStatisticsStack size(48):
class ml::maths::CLogJointProbabilityOfLessLikelySamples size(48):
class ml::maths::CLogJointProbabilityOfLessLikelySamples size(56):
class ml::maths::CTimeSeriesDecomposition size(664):
class ml::maths::CTimeSeriesDecomposition size(672):
class ml::maths::`anonymous-namespace'::detail::CHashIterator size(16):
class ml::maths::`anonymous-namespace'::detail::CHashIterator size(8):
class ml::model::CHierarchicalResultsNormalizer size(200):

CHierarchicalResultsNormalizer only appears once in that list because although its layout changes its overall size doesn't - there's already padding and the padding just gets shuffled to different places.

So, the list of classes that can benefit from __declspec(empty_bases) is:

  1. ml::maths::CBasicStatistics::COrderStatisticsHeap
  2. ml::maths::CBasicStatistics::COrderStatisticsStack
  3. ml::maths::CLogJointProbabilityOfLessLikelySamples
  4. ml::maths::CTimeSeriesDecomposition
  5. ml::maths::`anonymous-namespace'::detail::CHashIterator

Cool stuff!

droberts195 added a commit to droberts195/ml-cpp that referenced this pull request Nov 22, 2019
The empty base optimization is not used by default with
multiple inheritance on Windows, but can be enabled using
__declspec(empty_bases).  The investigation in elastic#842 shows
which of our classes would benefit, so this PR adds the
required option to the 5 affected classes.
@droberts195
Copy link
Contributor Author

#844 has the fixes.

droberts195 added a commit that referenced this pull request Nov 22, 2019
The empty base optimization is not used by default with
multiple inheritance on Windows, but can be enabled using
__declspec(empty_bases).  The investigation in #842 shows
which of our classes would benefit, so this PR adds the
required option to the 5 affected classes.
droberts195 added a commit to droberts195/ml-cpp that referenced this pull request Nov 22, 2019
The empty base optimization is not used by default with
multiple inheritance on Windows, but can be enabled using
__declspec(empty_bases).  The investigation in elastic#842 shows
which of our classes would benefit, so this PR adds the
required option to the 5 affected classes.

Backport of elastic#844
droberts195 added a commit that referenced this pull request Nov 22, 2019
The empty base optimization is not used by default with
multiple inheritance on Windows, but can be enabled using
__declspec(empty_bases).  The investigation in #842 shows
which of our classes would benefit, so this PR adds the
required option to the 5 affected classes.

Backport of #844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants