[PATCH] D20914: [esan|cfrag] Compute the struct field access variance

[Qin Zhao via llvm-commits]

zhaoqin added inline comments.

================
Comment at: lib/esan/cache_frag.cpp:62
@@ +61,3 @@
+
+static u32 computeVarianceScore(u64 Val1, u64 Val2) {
+  u64 Ratio;
----------------
aizatsky wrote:
> aizatsky wrote:
> > As far as I can see you compute this:
> > 
> > Log[t, v1 / (v2 + 1) / t]
> > 
> > Is this right?
> > 
> > This is equivalent to:
> > 
> > Log[t, v1/(v2+1)]-1
> > 
> > So lets:
> > 
> > a) document this formula
> > b) maybe get rid of this -1? Would simplify things.
> If my assumption about formula is right, then how about logratio as a metric name?
changed the algorithm a bit with an added weight, not sure if logratio is still suitable.

================
Comment at: lib/esan/cache_frag.cpp:62
@@ +61,3 @@
+
+static u32 computeVarianceScore(u64 Val1, u64 Val2) {
+  u64 Ratio;
----------------
zhaoqin wrote:
> aizatsky wrote:
> > aizatsky wrote:
> > > As far as I can see you compute this:
> > > 
> > > Log[t, v1 / (v2 + 1) / t]
> > > 
> > > Is this right?
> > > 
> > > This is equivalent to:
> > > 
> > > Log[t, v1/(v2+1)]-1
> > > 
> > > So lets:
> > > 
> > > a) document this formula
> > > b) maybe get rid of this -1? Would simplify things.
> > If my assumption about formula is right, then how about logratio as a metric name?
> changed the algorithm a bit with an added weight, not sure if logratio is still suitable.
add comment about the formula.
The reason I have -1 is to make the score 0 if the two values are similar, so it is still 0 even with adding some weight, add comment.


================
Comment at: lib/esan/cache_frag.cpp:66
@@ +65,3 @@
+  if (Val1 > Val2)
+    Ratio = Val1 / (Val2 + 1) / Threshold;
+  else
----------------
aizatsky wrote:
> Why + 1? Because of 0 values? 
> 
> What do you imagine computeVarianceScore(v1, 0, t) look like?
Yes, it is for handling 0.
for computeVarianceScore(v1, 0, t), it should be roughly the same as computeVarianceScore(v1, 1, t)
The rationale of computeVarianceScore is to evaluate how far apart the two value are.
so the variance score for (v1, 0) should be closest to the score of (v1, 1).
Use condition instead.

================
Comment at: lib/esan/cache_frag.cpp:108
@@ +107,3 @@
+    Handle->Count += Handle->Struct->FieldCounters[i];
+    Handle->Variance += computeVarianceScore(
+        Handle->Struct->FieldCounters[i - 1], Handle->Struct->FieldCounters[i]);
----------------
aizatsky wrote:
> I'm not sure that adding log-ratios together is a good thing to do. Do you have any reasoning why you do it this way?
You are right, I add weight in the computeVarianceScore, so it is no longer a simple log-ratios.


http://reviews.llvm.org/D20914