Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQZ_apply_differences [CORE3148] #3525

Closed
firebird-automations opened this issue Sep 27, 2010 · 9 comments
Closed

SQZ_apply_differences [CORE3148] #3525

firebird-automations opened this issue Sep 27, 2010 · 9 comments

Comments

@firebird-automations
Copy link
Collaborator

Submitted by: @ibprovider

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]

Commits: 19e53d3 ffd0aaa 15132bc 88e0503

@firebird-automations
Copy link
Collaborator Author

Modified by: @AlexPeshkoff

assignee: Alexander Peshkov [ alexpeshkoff ]

@firebird-automations
Copy link
Collaborator Author

Commented by: Ann Harrison (awharrison)

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.

@firebird-automations
Copy link
Collaborator Author

Commented by: @AlexPeshkoff

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.

@firebird-automations
Copy link
Collaborator Author

Commented by: @AlexPeshkoff

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.

@firebird-automations
Copy link
Collaborator Author

Commented by: @AlexPeshkoff

Expected performace drawback is so small taht I'ev found it's possible to use original solution

@firebird-automations
Copy link
Collaborator Author

Modified by: @AlexPeshkoff

status: Open [ 1 ] => Resolved [ 5 ]

resolution: Fixed [ 1 ]

Fix Version: 2.1.4 [ 10361 ]

Fix Version: 2.5.1 [ 10333 ]

Fix Version: 3.0 Alpha 1 [ 10331 ]

Fix Version: 2.0.7 [ 10390 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @pcisar

status: Resolved [ 5 ] => Closed [ 6 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

QA Status: No test

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

status: Closed [ 6 ] => Closed [ 6 ]

QA Status: No test => Cannot be tested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment