https://codereview.chromium.org/110203002/diff/20001/src/accessors.cc
File src/accessors.cc (right):
https://codereview.chromium.org/110203002/diff/20001/src/accessors.cc#newcode653
src/accessors.cc:653: if (!Compiler::EnsureCompiled(function_handle,
KEEP_EXCEPTION)) {
Not sure why you negated this condition, can you switch it back?
https://codereview.chromium.org/110203002/diff/20001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):
https://codereview.chromium.org/110203002/diff/20001/src/arm/builtins-arm.cc#newcode292
src/arm/builtins-arm.cc:292: static void
CallRuntimePassFunctionAndTailCall(
Maybe split this into a CallRuntimeWithFunction and then another method,
TailCall, since we also tail call in CallCompileOptimizedAndTailCall
below...
https://codereview.chromium.org/110203002/diff/20001/src/arm/builtins-arm.cc#newcode780
src/arm/builtins-arm.cc:780: { FrameScope scope(masm,
StackFrame::INTERNAL);
This is an almost exact copy of CallRuntimePassFunctionAndTailCall, but
with a known function id and an extra parameter...
https://codereview.chromium.org/110203002/diff/20001/src/bootstrapper.cc
File src/bootstrapper.cc (right):
https://codereview.chromium.org/110203002/diff/20001/src/bootstrapper.cc#newcode1515
src/bootstrapper.cc:1515: script_name, 0, 0,
formatting changes on this line.
https://codereview.chromium.org/110203002/diff/20001/src/builtins.h
File src/builtins.h (right):
https://codereview.chromium.org/110203002/diff/20001/src/builtins.h#newcode103
src/builtins.h:103: V(LazyCompile, BUILTIN,
UNINITIALIZED, \
s/LazyCompile/Compile/
?
https://codereview.chromium.org/110203002/diff/20001/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/110203002/diff/20001/src/compiler.cc#newcode349
src/compiler.cc:349: if
(!info()->closure()->PassesFilter(FLAG_hydrogen_filter)) {
Do we also want to set a bailout reason and disable optimization here?
I think we probably always want to pass the bailout reason to
Abort*Optimization()
https://codereview.chromium.org/110203002/diff/20001/src/compiler.cc#newcode1215
src/compiler.cc:1215: ASSERT(info->shared_info()->scope_info() !=
ScopeInfo::Empty(isolate));
Don't know why this assert is here.
https://codereview.chromium.org/110203002/diff/20001/src/compiler.h
File src/compiler.h (right):
https://codereview.chromium.org/110203002/diff/20001/src/compiler.h#newcode506
src/compiler.h:506: class RecompileJob: public ZoneObject {
Probably want to rename this class as well. Maybe OptimizedCompileJob?
https://codereview.chromium.org/110203002/diff/20001/src/factory.cc
File src/factory.cc (right):
https://codereview.chromium.org/110203002/diff/20001/src/factory.cc#newcode765
src/factory.cc:765: result->MarkForCompileOptimized();
MarkForCompileOptimized reads weird...why not mark for optimization?
Btw, why do we check result->is_compiled() here?
https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.cc
File src/full-codegen.cc (right):
https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.cc#newcode1715
src/full-codegen.cc:1715: for (uint32_t i = 0; i < back_edges.length();
i++) {
I'm not so sure about the assumption that the AST ids have a unique
entry in the backedge table, which appears to be already assumed in the
previous code.
https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.cc#newcode1722
src/full-codegen.cc:1722: ASSERT(Verify(isolate, code,
code->allow_osr_at_loop_nesting_level()));
Probably not going to work if you have a recursive function, directly or
indirectly, that is OSR'd.
https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h
File src/full-codegen.h (right):
https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h#newcode931
src/full-codegen.h:931: static void AddStackCheck(Code* code, uint32_t
pc_offset);
Why not a Handle<Code>?
https://codereview.chromium.org/110203002/diff/20001/src/full-codegen.h#newcode934
src/full-codegen.h:934: static void RemoveStackCheck(CompilationInfo*
info);
And why does this one take a CompilationInfo? Does it remove all checks,
or just one?
https://codereview.chromium.org/110203002/diff/20001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):
https://codereview.chromium.org/110203002/diff/20001/src/ia32/builtins-ia32.cc#newcode77
src/ia32/builtins-ia32.cc:77: static void
CallRuntimePassFunctionAndTailCall(
Similar comments to the arm port on naming here.
https://codereview.chromium.org/110203002/diff/20001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9474
src/objects.cc:9474: ASSERT(is_compiled() ||
GetIsolate()->DebuggerHasBreakPoints());
I don't know why we assert is_compiled()
https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9486
src/objects.cc:9486: ASSERT(is_compiled() ||
GetIsolate()->DebuggerHasBreakPoints());
Same.
https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9487
src/objects.cc:9487: ASSERT(!IsOptimized());
Or this.
https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9489
src/objects.cc:9489: ASSERT(!shared()->is_generator());
Or that.
https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9594
src/objects.cc:9594: ASSERT(index > kEntriesStart);
=?
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8307
src/runtime.cc:8307: PrintF("[lazy: ");
s/lazy/unoptimized/
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8314
src/runtime.cc:8314: ASSERT(!function->is_compiled());
Why?
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8334
src/runtime.cc:8334: if (!function->shared()->is_compiled()) {
I don't think we want this check. IIRC that forces all functions to be
first compiled unoptimized and then recompiled.
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8356
src/runtime.cc:8356: if (code.is_null()) code =
isolate->builtins()->InOptimizationQueue();
I totally don't understand this. What if the optimized compile just
fails (without recompilation on)? What is it supposed to return?
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8610
src/runtime.cc:8610: Handle<Code>
current_code(function->shared()->code());
Let's call this caller_code?
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode12686
src/runtime.cc:12686: static const ParseRestriction restriction =
NO_PARSE_RESTRICTION;
Why the extra local here?
https://codereview.chromium.org/110203002/diff/20001/test/cctest/test-compiler.cc
File test/cctest/test-compiler.cc (right):
https://codereview.chromium.org/110203002/diff/20001/test/cctest/test-compiler.cc#newcode108
test/cctest/test-compiler.cc:108: Handle<String>(), 0, 0,
formatting changes
https://codereview.chromium.org/110203002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
