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
