Issue Details (XML | Word | Printable)

Key: CORE-3148
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Alexander Peshkov
Reporter: Kovalenko Dmitry
Votes: 0
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
Firebird Core

SQZ_apply_differences

Created: 27/Sep/10 04:47 AM   Updated: 04/Feb/11 12:21 PM
Component/s: Engine
Affects Version/s: 2.5 RC3
Fix Version/s: 2.1.4, 2.5.1, 2.0.7, 3.0 Alpha 1

Time Tracking:
Not Specified

Planning Status: Unspecified


 Description  « Hide
Current code:
------------------ [BEGIN]
while (differences < end && p < p_end)
{
const SSHORT l = *differences++;
if (l > 0)
{
if (p + l > p_end)
{
BUGCHECK(177); /* msg 177 applied differences will not fit in record */
}
memcpy(p, differences, l); // <--------- CAN READ AFTER LAST ELEMENT OF differences
------------------ [END]

I offer to rewrite this function

------------------ [BEGIN]
USHORT SQZ_apply_differences(Record* const record, const SCHAR* differences, const SCHAR* const end)
{
/**************************************
 *
 * S Q Z _ a p p l y _ d i f f e r e n c e s
 *
 **************************************
 *
 * Functional description
 * Apply a differences (delta) to a record. Return the length.
 *
 **************************************/
typedef USHORT result_type;

fb_assert(record != NULL);
fb_assert(differences <= end);

if (end - differences > MAX_DIFFERENCES)
{
BUGCHECK(176); /* msg 176 bad difference record */
}

SCHAR* p = reinterpret_cast<SCHAR*>(record->rec_data);

const SCHAR* const p_beg = p;
const SCHAR* const p_end = p_beg + record->rec_length;

while (differences < end)
{
const SSHORT l = *differences;

++differences;

//fb_assert(differences <= end);
fb_assert(p >= p_beg);
fb_assert(p <= p_end);

if (l > 0)
{
if (static_cast<size_t>(p_end - p) < static_cast<size_t>(l))
{
BUGCHECK(177); /* msg 177 applied differences will not fit in record */
}

if (static_cast<size_t>(end - differences) < static_cast<size_t>(l)) //<---- an important test
{
BUGCHECK(177); /* or 176? */
}

memcpy(p, differences, l);
p += l;
differences += l;
}
else
{
if (static_cast<size_t>(p_end - p) < static_cast<size_t>(-l))
{
BUGCHECK(177); /* msg 177 applied differences will not fit in record */
}

p += -l;
}//else
}//while (differences < end)

fb_assert(differences == end);
fb_assert(p >= p_beg);
fb_assert(p <= p_end);

//fb_assert(can_numeric_cast<result_type>(p - p_beg));

const result_type length = static_cast<result_type>(p - p_beg);

fb_assert(length <= record->rec_length);

return length;
}//SQZ_apply_differences
------------------ [END]


 All   Comments   Work Log   Change History   Version Control   Subversion Commits      Sort Order: Ascending order - Click to sort in descending order
Ann Harrison added a comment - 27/Sep/10 02:57 PM
That is about the most commonly executed code in Firebird - I suspect your
changes, however clearer and possibly more correct, will reduce performance
measurably unless the c++ compilers have improved enormously. Do look
at the generated code and test performance before you make that change.

Alexander Peshkov added a comment - 28/Sep/10 06:40 AM
Ann, I understand your worries about performance. And I do not believe in compilers that produce same efficiency code with added conditional check inside the loop:-) Unfortunately without suggested by Dmitry test we have real possibility of segfault when working with corrupted database. Imagine end of differences exactly at the end of OS-allocated memory segment. If we read after the end, we get segfault.
This was not too big problem in classic-only server times (connection lost message instead corrupted database), but for MT engine this is definitely serious. As another possible solution for that test we can expand memory, allocated for data pages cache (differences are taken from this place, am I right?), with big enough tail, which can even be used for some other purpose (we only read it in the worst case). Unfortunately, I have no idea at once, how big must be that tail. May be maximum record size? Appears we can't read more data from differences. If yes, that solution IMO preferred.

Alexander Peshkov added a comment - 19/Oct/10 01:02 PM
As was expected, this fix makes test, specially oriented on massive differences applies, work slower - 0.17%. Taking into an account that deviation is series of test is 5 times greater and this is artificial test, I suppose patch can be applied.

Alexander Peshkov added a comment - 19/Oct/10 01:46 PM
Expected performace drawback is so small taht I'ev found it's possible to use original solution