[ltt-dev] [PATCH] Fixing bugs in cluster synchronisation for accuracy algorithm

[benjamin.poirier@polymtl.ca (Benjamin Poirier)]

On 13/09/10 04:39 PM, Mathieu Desnoyers wrote:
> * masoume.jabbarifar at polymtl.ca (masoume.jabbarifar at polymtl.ca) wrote:
>> From: Masoume Jabbarifar<masoume.jabbarifar@polymtl.ca>
>>
>
> Can you document the change in the patch header ?
>
> Also, please CC Benjamin on these changes. I'd like him to have a look
> at them if he has time.
>
>> ---
>>   lttv/lttv/sync/data_structures.h           |    2 +-
>>   lttv/lttv/sync/factor_reduction_accuracy.c |   15 ++++++++++++---
>>   lttv/lttv/sync/sync_chain_lttv.c           |   18 ++++++++++++------
>>   lttv/lttv/sync/sync_chain_unittest.c       |    9 ++++++---
>>   4 files changed, 31 insertions(+), 13 deletions(-)
>>
>> diff --git a/lttv/lttv/sync/data_structures.h b/lttv/lttv/sync/data_structures.h
>> index 627286c..2206455 100644
>> --- a/lttv/lttv/sync/data_structures.h
>> +++ b/lttv/lttv/sync/data_structures.h
>> @@ -131,7 +131,7 @@ typedef struct
>>    * synchronization */
>>   typedef struct
>>   {
>> -	double drift, offset;
>> +	double drift, offset, accuracyToRefNode;
>
> Hrm, I see that the sync code use a different coding style than the rest
> of lttv. *sight* OK then.

Yeah, I know... I used my own style. Given that the coding style in lttv 
is pretty loose in some places I didn't feel too guilty ;)

Concerning style, if this patch is to be consistent with the rest of the 
sync code:
* put a space only to the right of assignment operators ('=', '+=', ...).
* put a space on each side of other operators
* put a space after commas (',')

Putting a space only to the right of the assignment operators has helped 
me catch a few 'if (x= 0)' bugs. In any case, this patch in itself has 
inconsistent styling. I've outlined the first occurrence where each rule 
could apply in the patch.

>
> For your information, the rest of lttv uses lower cases for variable
> identifiers, and UpperCase for type identifiers. It also uses
> lower_case_with_underscore() for function names.
>
>>   } Factors;
>>
>>   // Correction factors between trace pairs, to be used for reduction
>> diff --git a/lttv/lttv/sync/factor_reduction_accuracy.c b/lttv/lttv/sync/factor_reduction_accuracy.c
>> index a63dae7..bf0546b 100644
>> --- a/lttv/lttv/sync/factor_reduction_accuracy.c
>> +++ b/lttv/lttv/sync/factor_reduction_accuracy.c
>> @@ -415,12 +415,19 @@ static void getFactors(AllFactors* const allFactors, unsigned int** const
>>   	unsigned int reference;
>>   	PairFactors** const pairFactors= allFactors->pairFactors;
>>
>> -	reference= references[traceNum];
>> -
>> +	if (traceNum == references[traceNum])
>> +	{
>> +		reference=traceNum;

No space around the '='

>> +	}
>> +	else
>> +	{
>> +		reference = predecessors[references[traceNum]][traceNum];

Spaces on both sides of the '='

>> +	}
>>   	if (reference == traceNum)
>>   	{
>>   		factors->offset= 0.;
>>   		factors->drift= 1.;
>> +		factors->accuracyToRefNode= 0.;

The spacing is right on that one.

The field should not be added to this struct however but in an array in 
struct ReductionStatsAccuracy. The information that field holds is 
specific to the "accuracy" reduction module and is only useful if the 
user wants extra output about the innards of the synchronization 
process. It's not useful in the general case where the user simply wants 
to look at synchronized traces.

>>   	}
>>   	else
>>   	{
>> @@ -428,6 +435,8 @@ static void getFactors(AllFactors* const allFactors, unsigned int** const
>>
>>   		getFactors(allFactors, predecessors, references,
>>   			predecessors[reference][traceNum],&previousVertexFactors);
>> +		factors->accuracyToRefNode =
>> +			pairFactors[reference][traceNum].accuracy+ pairFactors[traceNum][reference].accuracy + previousVertexFactors.accuracyToRefNode;

Please put a space on each side of the first '+'

>>
>>   		/* Convert the time from traceNum to reference;
>>   		 * pairFactors[row][col] converts the time from col to row, invert the
>> @@ -452,7 +461,7 @@ static void getFactors(AllFactors* const allFactors, unsigned int** const
>>   		}
>>   		else
>>   		{
>> -			g_assert_not_reached();
>> +			//g_assert_not_reached();
>
> Why commenting this ? This assertion was probably there for a reason.
> Shutting up assertions without proper comments is usually a bad
> practice, as assertions are our safety nets.
>
>>   		}
>>   	}
>>   }
>> diff --git a/lttv/lttv/sync/sync_chain_lttv.c b/lttv/lttv/sync/sync_chain_lttv.c
>> index 95bef44..20ed111 100644
>> --- a/lttv/lttv/sync/sync_chain_lttv.c
>> +++ b/lttv/lttv/sync/sync_chain_lttv.c
>> @@ -218,6 +218,7 @@ bool syncTraceset(LttvTracesetContext* const traceSetContext)
>>   	double minOffset, minDrift;
>>   	unsigned int refFreqTrace;
>>   	int retval;
>> +	double sumAccuracy;
>>
>>   	if (!optionSync.present)
>>   	{
>> @@ -370,8 +371,6 @@ bool syncTraceset(LttvTracesetContext* const traceSetContext)
>>   					t->drift * t->start_tsc + t->offset));
>>   	}
>>
>> -	g_array_free(factors, TRUE);
>> -
>>   	lttv_traceset_context_compute_time_span(traceSetContext,
>>   		&traceSetContext->time_span);
>>
>> @@ -395,21 +394,28 @@ bool syncTraceset(LttvTracesetContext* const traceSetContext)
>>   	if (!optionSyncNull.present&&  optionSyncStats.present)
>>   	{
>>   		printStats(syncState);
>> -
>> +		sumAccuracy = 0;
>
>    = 0.;
>
>>   		printf("Resulting synchronization factors:\n");
>>   		for (i= 0; i<  syncState->traceNb; i++)
>>   		{
>>   			LttTrace* t;
>> +			Factors* traceFactors;
>>
>>   			t= traceSetContext->traces[i]->t;
>> -
>> -			printf("\ttrace %u drift= %g offset= %g (%f) start time= %ld.%09ld\n",
>> +			traceFactors=&g_array_index(factors, Factors, i);
>> +			
>> +			printf("\ttrace %u drift= %g offset= %g (%f) start time= %ld.%09ld accuracyToRefNode= %g\n",

Same thing regarding specificity to the "accuracy" reduction module. All 
of this printing should be done in printReductionStatsAccuracy().

>>   				i, t->drift, t->offset, (double) tsc_to_uint64(t->freq_scale,
>>   					t->start_freq, t->offset) / NANOSECONDS_PER_SECOND,
>>   				t->start_time_from_tsc.tv_sec,
>> -				t->start_time_from_tsc.tv_nsec);
>> +				t->start_time_from_tsc.tv_nsec,traceFactors->accuracyToRefNode);
>> +			sumAccuracy += traceFactors->accuracyToRefNode;
>> +
>>   		}
>>   	}
>> +	printf("Total accuracy (0.0 is the best): %g\n",sumAccuracy);

Please put a space after the comma

>> +
>> +	g_array_free(factors, TRUE);
>>
>>   	syncState->processingModule->destroyProcessing(syncState);
>>   	if (syncState->matchingModule != NULL)
>> diff --git a/lttv/lttv/sync/sync_chain_unittest.c b/lttv/lttv/sync/sync_chain_unittest.c
>> index 40302a0..d59eb09 100644
>> --- a/lttv/lttv/sync/sync_chain_unittest.c
>> +++ b/lttv/lttv/sync/sync_chain_unittest.c
>> @@ -114,6 +114,7 @@ int main(const int argc, char* const argv[])
>>   	GString* analysisModulesNames;
>>   	GString* reductionModulesNames;
>>   	unsigned int id;
>> +	double sumAccuracy;
>>   	AllFactors* allFactors;
>>
>>   	/*
>> @@ -266,15 +267,17 @@ int main(const int argc, char* const argv[])
>>   		unsigned int i;
>>
>>   		printStats(syncState);
>> -
>> +		sumAccuracy = 0;
>
>   = 0.;
>
> Thanks,
>
> Mathieu
>
>>   		printf("Resulting synchronization factors:\n");
>>   		for (i= 0; i<  factors->len; i++)
>>   		{
>>   			Factors* traceFactors=&g_array_index(factors, Factors, i);
>> -			printf("\ttrace %u drift= %g offset= %g\n", i,
>> -				traceFactors->drift, traceFactors->offset);
>> +			printf("\ttrace %u drift= %g offset= %g accuracyToRefNode= %g\n", i,
>> +				traceFactors->drift, traceFactors->offset, traceFactors->accuracyToRefNode);
>> +			sumAccuracy += traceFactors->accuracyToRefNode;
>>   		}
>>   	}
>> +	printf("Total accuracy (0.0 is the best): %g\n",sumAccuracy);

Printing it in printReductionStatsAccuracy() would avoid this duplication.

-Ben

>>
>>   	// Destroy modules and clean up
>>   	syncState->processingModule->destroyProcessing(syncState);
>> --
>> 1.6.0.4
>>
>>
>> _______________________________________________
>> ltt-dev mailing list
>> ltt-dev at lists.casi.polymtl.ca
>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>
>