Inadvertently passing large objects by value

Raymond Chen

Raymond

One mark of punctuation can make all the difference.

One program was encountering a stack overflow exception in
a function that didn’t appear to be doing anything particularly
stack-hungry.
The following code illustrates the problem:

bool TestResults::IsEqual(TestResults& expected)
{
 if (m_testMask != expected.m_testMask) {
  return false;
 }
 bool result = true;
 if (result && (m_testMask & AbcTestType)) {
  result = CompareAbc(expected);
 }
 if (result && (m_testMask & DefTestType)) {
  result = CompareDef(expected);
 }
 if (result && (m_testMask & GhiTestType)) {
  result = CompareGhi(expected);
 }
 if (result && (m_testMask & JklTestType)) {
  result = CompareJkl(expected);
 }
 return result;
}

(In reality, the algorithm for comparing two tests results
was much more complicated, but that’s irrelevant to this
discussion.)

And yet on entry to this function, a stack overflow was raised.

The first thing to note is that this problem occurred only
on the x64 build of the test.
The x86 version ran fine, or at least appeared to.
It so happens that the x64 compiler aggressively inlines functions,
which as it turned out was a major exacerbator of the problem.

The title of this entry probably tipped you off to what happened:
The helper functions accepted the test results parameter by value
not by reference:

bool TestResults::CompareAbc(TestResults expected);
bool TestResults::CompareDef(TestResults expected);
bool TestResults::CompareGhi(TestResults expected);
bool TestResults::CompareJkl(TestResults expected);

and those comparison functions in turn called other
comparison functions, which also passed the TestResults
by value.
Since the test results were passed by value, a temporary
copy was made on the stack and passed to the comparison function.
It so happened that the TestResults class was
a very large one, a hundred kilobytes or so, and the
TestResults::IsEqual function therefore needed
to reserve room for a large number of such temporary copies,
one for each call to a comparison function in each of the
inlined functions.
A dozen temporary copies times a hundred kilobytes per copy
comes out to over a megabyte of temporary variables,
which exceeded the default one megabyte stack size
and therefore resulted in a stack overflow exception on entry
to the TestResults::IsEqual function.

This code appeared to run fine when compiled for the x86 architecture
because the x86-targetting compiler did not inline quite as
aggressively, so the large temporaries were not reserved on the
stack until the helper comparison was actually called.
Since the comparisons went only three levels deep,
there were only three temporary copies of the
expected parameter, which fit within the one megabyte
default stack.
It was still bad code—consuming a few hundred kilobytes
of stack for no reason—but it wasn’t bad enough to cause
a problem.
The fix, of course, was to change the comparison functions to
accept the parameter by reference.

bool TestResults::IsEqual(const TestResults& expected) const;
bool TestResults::CompareAbc(const TestResults& expected) const;
bool TestResults::CompareDef(const TestResults& expected) const;
bool TestResults::CompareGhi(const TestResults& expected) const;
bool TestResults::CompareJkl(const TestResults& expected) const;

For good measure, the parameter was changed to a
const reference, and the function was
tagged as itself const to emphasize that
neither the object nor the expected value will be modified
as part of the comparison,
thereby ensuring that changing from a copy to a const reference
didn’t change the previous behavior.
Without the const reference,
there was a possibility that somewhere deep inside the comparison
functions, they made a change to the expected
parameter.
Under the old pass-by-value declaration, this change was discarded
when the function returned since the change was made to a copy.
If we had left off the const from the reference,
then we would have changed the behavior: The change to the
expected parameter would have modified the original
TestResults.
Making the parameter const reassures us that an
attempt to modify expected would be flagged by the
compiler and therefore brought to our attention.

(This technique is not foolproof, however.
Somebody could always cast away const-ness and
modify the original,
but we were being reckless and assuming that nobody would be that
crazy.)

Raymond Chen
Raymond Chen

Follow Raymond   

0 comments

Comments are closed.