harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Egor Pasko <egor.pa...@gmail.com>
Subject Re: [drlvm][em] Value-profiling implementation is avaible.
Date Tue, 31 Oct 2006 08:35:02 GMT
On the 0x212 day of Apache Harmony Yuri Kashnikoff wrote:
> Hi!
> I'am working on value-profiling implementation. Implementation in EM
> is alredy avaible. Now I'am trying to implement and use it in JIT. I
> can answer any questions about current implementation. I have used the
> straigth algorithm (cacthing FIRST N values) as default and as
> advanced algorithm the Top-N-Value algorithm from the B. Calder, P.
> Feller "Value Profiling and Optimisation". I'll appreciate any
> questions. Please review and comment.

Yuri, I reviewed the patch (a little) and have something nerdy to say.

Pavel, sorry if it looks like stealing the interesting piece of code
in front of you :), but I am just to put a set of quick-and-simple
comments. Did not look in details. You can now focus on more in-depth
things.

Yuri, I really appreciate what you are diong (!!), 
and let's now become nerdy:

* line 30 of the patch: needs an extra empty line, otherwise does not apply
  (you edited the patch by hand?:)

* tabs should be converted to spaces (!!) (and overall indentation is
  *not* very beautiful) to be corrected, IMHO

* include <vector>? really need it? :)

* many lines are commented out (!!!) please, clean them up, or make it
  conditionally compiled, so the code looks cleaner, readable and
  supportable. 

* more comments are becoming the imminent desire of your single header

* dos2unix? :) I am a nerd :)

* NValueProfileCollector.cpp -> "no newline at end of file" -> build failed :)

* ValueProfileCollector.h:48 TNV_ENTIRE should be added here to
  compile (after this and the above suggested changes it at least compiles)

* we do not use 'int', use 'int32', 'uint32', 'int64', etc., please,
  it is a common convention within the DRLVM code

* you do one global lock (profilesLock), that would be a *real crawl* :)
  Profiling is method-wise, I'd suggest a lock per method. How is
  that?

* am I missing something here?
  ValueProfileCollector::addNewValue:
  //insert_into_tnv_table (last_value, num_times_profiled); to-do:
  (the only 'insert_blablabla' is commented-out, does it work?)

* do you have an .emprofile to test your implementation? Would have
  been great!

* why EM_PCTYPE updated, but ProfileType left as-is? is it intentional?

* why a value is a mere 'int'? what about pointer profiling? it is different
  on IA-32 and x86_64. How would you write a portable piece of
  profiler? Not nerdy, just curious :)

Yuri, could you, plz, update your 2 files and the patch according to
my observations? (and discuss here on what you do not agree with, of course:)

-- 
Egor Pasko, Intel Managed Runtime Division


Mime
View raw message