harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pavel Ozhdikhin" <pavel.ozhdik...@gmail.com>
Subject Re: [drlvm][em] Value-profiling implementation is avaible.
Date Tue, 31 Oct 2006 09:13:45 GMT
>
> 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.
>

Thanks, Egor, for such a thorough review.  You just have done the first part
of the work for me. :)
Actually, I'm more interested in practical use of value profiling framework.
I've discussed this with Mikhail and AFAIU to emply the value profiling we
need implementation of instrumentation/annotation passes for the profiler.
Not a big, but an importance piece.

Thanks,
-Pavel


On 31 Oct 2006 14:35:02 +0600, Egor Pasko <egor.pasko@gmail.com> wrote:
>
> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message