Addressed comments.
Regarding the is_compiled() asserts: we currently assume that we first
compile
unoptimized code before optimizing, and guard that in several places. I'd
like
to keep it this way for this CL. We may reconsider this later.
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)) {
On 2013/12/09 14:49:28, titzer wrote:
Not sure why you negated this condition, can you switch it back?
Done.
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(
On 2013/12/09 14:49:28, titzer wrote:
Maybe split this into a CallRuntimeWithFunction and then another
method,
TailCall, since we also tail call in CallCompileOptimizedAndTailCall
below...
Done.
https://codereview.chromium.org/110203002/diff/20001/src/arm/builtins-arm.cc#newcode780
src/arm/builtins-arm.cc:780: { FrameScope scope(masm,
StackFrame::INTERNAL);
On 2013/12/09 14:49:28, titzer wrote:
This is an almost exact copy of CallRuntimePassFunctionAndTailCall,
but with a
known function id and an extra parameter...
I thought it would make things unnecessarily complicated by making one
function that handles both. I did refactor out the tail call.
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)) {
On 2013/12/09 14:49:28, titzer wrote:
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()
Done.
https://codereview.chromium.org/110203002/diff/20001/src/compiler.cc#newcode1215
src/compiler.cc:1215: ASSERT(info->shared_info()->scope_info() !=
ScopeInfo::Empty(isolate));
On 2013/12/09 14:49:28, titzer wrote:
Don't know why this assert is here.
Removed.
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 {
On 2013/12/09 14:49:28, titzer wrote:
Probably want to rename this class as well. Maybe OptimizedCompileJob?
Done.
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();
On 2013/12/09 14:49:28, titzer wrote:
MarkForCompileOptimized reads weird...why not mark for optimization?
Btw, why do we check result->is_compiled() here?
Done. We only optimize if we have unoptimized code.
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++) {
On 2013/12/09 14:49:28, titzer wrote:
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.
AST ids in the back edge table are taken from
IterationStatement::OsrEntryId(). It would be weird to have the same OSR
entry assigned to two different iteration statements offsets. The
osr_entry_id_ value of the IterationStatement is set in the constructor
via GetNextId().
I can add some assertions to ensure this assumption, if it bothers you.
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()));
On 2013/12/09 14:49:28, titzer wrote:
Probably not going to work if you have a recursive function, directly
or
indirectly, that is OSR'd.
Verify only tests that the back edge states are consistent with the
allow_osr_at_loop_nesting_level value in that more deeply nested back
edges are not patched and vice versa. RemoveStackCheck does not change
this state: both OSR_AFTER_STACK_CHECK and ON_STACK_REPLACEMENT are
considered patched in Verify.
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#newcode934
src/full-codegen.h:934: static void RemoveStackCheck(CompilationInfo*
info);
On 2013/12/09 14:49:28, titzer wrote:
And why does this one take a CompilationInfo? Does it remove all
checks, or just
one?
This removes only the one added in AddStackCheck. All the necessary
information are in the compilation info, so I though passing that would
be cleaner.
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(
On 2013/12/09 14:49:28, titzer wrote:
Similar comments to the arm port on naming here.
Done.
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());
On 2013/12/09 14:49:28, titzer wrote:
I don't know why we assert is_compiled()
The assumption is that we compiled unoptimized code prior to optimized
code, unless the debugger threw away all code in between.
https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9487
src/objects.cc:9487: ASSERT(!IsOptimized());
On 2013/12/09 14:49:28, titzer wrote:
Or this.
To guard that we don't optimize something we already have optimized code
for (that hasn't deopted yet), in case this happens accidentally when we
change something.
https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9489
src/objects.cc:9489: ASSERT(!shared()->is_generator());
On 2013/12/09 14:49:28, titzer wrote:
Or that.
Generators are not optimizable, and we should have caught that earlier.
https://codereview.chromium.org/110203002/diff/20001/src/objects.cc#newcode9594
src/objects.cc:9594: ASSERT(index > kEntriesStart);
On 2013/12/09 14:49:28, titzer wrote:
>=?
Each entry of the optimized code map consists of native context, code
object, and literals array, in exactly that order. The first entry of an
optimized code map, at kEntriesStart, is therefore a native context.
The index argument is found via SearchOptimizedCodeMap, which returns
the index for the code object in the entry, which is at least
kEntries+1.
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: ");
On 2013/12/09 14:49:28, titzer wrote:
s/lazy/unoptimized/
Done.
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8314
src/runtime.cc:8314: ASSERT(!function->is_compiled());
On 2013/12/09 14:49:28, titzer wrote:
Why?
I guess we would guard that this is only called from the builtin. I
think we can remove this.
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8334
src/runtime.cc:8334: if (!function->shared()->is_compiled()) {
On 2013/12/09 14:49:28, titzer wrote:
I don't think we want this check. IIRC that forces all functions to be
first
compiled unoptimized and then recompiled.
I think that is intentional. I merely inlined AllowOptimization here. I
would like to keep this refactoring from changing the behavior to avoid
running into possible performance regressions. We could experiment with
this later.
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8356
src/runtime.cc:8356: if (code.is_null()) code =
isolate->builtins()->InOptimizationQueue();
On 2013/12/09 14:49:28, titzer wrote:
I totally don't understand this. What if the optimized compile just
fails
(without recompilation on)? What is it supposed to return?
You are right. Currently GetOptimizedCode would return the emtpy handle
for successfully queued optimization and the unoptimized shared code on
failure (so that it could be installed, as fallback).
I changed it so that it returns the empty handle on failure and the
builtin on successful queuing.
Also found a bug in case of concurrent OSR here.
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode8610
src/runtime.cc:8610: Handle<Code>
current_code(function->shared()->code());
On 2013/12/09 14:49:28, titzer wrote:
Let's call this caller_code?
Done.
https://codereview.chromium.org/110203002/diff/20001/src/runtime.cc#newcode12686
src/runtime.cc:12686: static const ParseRestriction restriction =
NO_PARSE_RESTRICTION;
On 2013/12/09 14:49:28, titzer wrote:
Why the extra local here?
Removed.
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,
On 2013/12/09 14:49:28, titzer wrote:
formatting changes
Done.
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.
