Thank you for the review. I have uploaded a new patch set.
On 2012/02/08 14:09:23, Erik Corry wrote:
The patch looks incomplete. Where is the ShouldSelfOptimize function?
Here: http://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/ast.cc#171 But I have moved it to CompilationInfo now (see comments below). https://chromiumcodereview.appspot.com/9361026/diff/1/src/full-codegen.cc File src/full-codegen.cc (right): https://chromiumcodereview.appspot.com/9361026/diff/1/src/full-codegen.cc#newcode300 src/full-codegen.cc:300: if (FLAG_counting_profiler) { On 2012/02/08 14:13:30, Erik Corry wrote:
I suggest this condition could be folded into NotifyCodeGenerated
Done. https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (right): https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-ia32.cc#newcode135 src/ia32/full-codegen-ia32.cc:135: if (FLAG_counting_profiler && On 2012/02/08 14:09:23, Erik Corry wrote:
These two flags can be tested inside ShouldSelfOptimize for a
reduction in
verbosity. Perhaps the flags on the shared info can also be tested
there? Good idea to clean this up. I've moved ShouldSelfOptimize() to CompilationInfo, where all the required information for this decision is accessible. https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-ia32.cc#newcode151 src/ia32/full-codegen-ia32.cc:151: Immediate(Smi::FromInt(1))); On 2012/02/08 14:09:23, Erik Corry wrote:
It seems you can do:
__ sub(Operand::Cell(Handle<JSGlobalPropertyCell>(cell)),
Immediate(...)) Done. https://chromiumcodereview.appspot.com/9361026/diff/1/src/ia32/full-codegen-ia32.cc#newcode154 src/ia32/full-codegen-ia32.cc:154: __ j(zero, compile_stub); On 2012/02/08 14:09:23, Erik Corry wrote:
There is an assumption here that the Smi tag is zero and that we
decrement one
at a time. I would prefer j(mi, ... instead.
I've added an ASSERT for the Smi tag. I can't easily change the jump condition because we intentionally don't reset the countdown. After it has fired, should we ever come back (e.g. after deopt), we simply ignore it (until it overflows and reaches zero again after 2 billion executions). https://chromiumcodereview.appspot.com/9361026/diff/1/src/ic-inl.h File src/ic-inl.h (right): https://chromiumcodereview.appspot.com/9361026/diff/1/src/ic-inl.h#newcode94 src/ic-inl.h:94: if (FLAG_counting_profiler) { On 2012/02/08 14:09:23, Erik Corry wrote:
Suggested comment:
// We do not want to optimize until the ICs have settled down // so when they are patched, we postpone optimization for the current
function
// and the functions above it on the stack that might want to // inline this one.
Done. https://chromiumcodereview.appspot.com/9361026/diff/1/src/objects.h File src/objects.h (right): https://chromiumcodereview.appspot.com/9361026/diff/1/src/objects.h#newcode5388 src/objects.h:5388: kThisPropertyAssignmentsOffset + kPointerSize; On 2012/02/08 14:09:23, Erik Corry wrote:
This is a Smi field. Can we find another one and put them both in one
field on
64 bit like we do for the ones below?
I'd love to, but I don't see any other Smi fields. Don't worry, though, there will be more in the future :-) https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc File src/runtime-profiler.cc (right): https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc#newcode50 src/runtime-profiler.cc:50: // Constants for old profiler. On 2012/02/08 14:09:23, Erik Corry wrote:
s/old/statistical/
Done. https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc#newcode63 src/runtime-profiler.cc:63: // Constants for new profiler. On 2012/02/08 14:09:23, Erik Corry wrote:
s/new/counter based/
Done. (However, given that this profiler is in its infancy, I'm not sure that "counter based" will be its most precise characterization. I kinda liked the flexibility in meaning that "new" provides ;-) https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc#newcode71 src/runtime-profiler.cc:71: static const int kSizeLimitEarlyOpt = 500; On 2012/02/08 14:09:23, Erik Corry wrote:
Name should contain 'Max'
Done. https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.cc#newcode238 src/runtime-profiler.cc:238: int threshold = function->shared()->ast_node_count() > 300 On 2012/02/08 14:09:23, Erik Corry wrote:
constant needs a name
Done. (By removing it -- it's not clear yet whether this additional condition causes any improvement.) https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.h File src/runtime-profiler.h (right): https://chromiumcodereview.appspot.com/9361026/diff/1/src/runtime-profiler.h#newcode66 src/runtime-profiler.h:66: } On 2012/02/08 14:09:23, Erik Corry wrote:
Missing blank line.
Done. https://chromiumcodereview.appspot.com/9361026/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
