addressed comments
https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode972
src/compiler.cc:972: ASSERT(osr_pc_offset != 0 ||
closure->IsMarkedForConcurrentRecompilation());
On 2013/09/03 14:05:36, titzer wrote:
Not sure what you are trying to assert here.
Asserting that if we are not recompiling for OSR, we get here via the
ConcurrentRecompile builtin. It's not absolutely necessary, so I can
remove it if it bothers you.
https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode994
src/compiler.cc:994: BailoutId osr_ast_id =
shared->code()->TranslatePcOffsetToAstId(
On 2013/09/03 14:05:36, titzer wrote:
Are you calling this method in another place? Otherwise, you're
passing in a
pointer to a loop_depth variable just to get it out for printing. That
seems
weird. Pointers to local variables should be avoided.
Done.
https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1121
src/compiler.cc:1121: Handle<Code> unoptimized) {
On 2013/09/03 14:05:36, titzer wrote:
Please put the mechanism for finding the osr_pc_offset back into the
runtime.
I got a CL in queue that removes this completely. Can we leave this here
for now?
https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1154
src/compiler.cc:1154: BailoutId* ast_id) {
On 2013/09/03 14:05:36, titzer wrote:
This should take the osr_pc_offset as a parameter.
Ditto. CL queued up doing exactly that.
https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1174
src/compiler.cc:1174: *ast_id =
unoptimized->TranslatePcOffsetToAstId(pc_offset, &loop_depth);
On 2013/09/03 14:05:36, titzer wrote:
If you move this code back to the runtime, you don't need to pass
pointers to a
BailoutId around.
I changed this so that we return the AST id directly. There is no need
for a bool return value.
https://codereview.chromium.org/23710014/diff/7001/src/compiler.cc#newcode1245
src/compiler.cc:1245: if (!function->IsOptimized()) {
On 2013/09/03 14:05:36, titzer wrote:
You shouldn't check function->IsOptimized, but look at the result of
the code,
which should be in the compilation info. Soon, OSR compilations won't
install
the code into the function.
Done.
https://codereview.chromium.org/23710014/diff/7001/test/mjsunit/compiler/optimized-for-in.js
File test/mjsunit/compiler/optimized-for-in.js (right):
https://codereview.chromium.org/23710014/diff/7001/test/mjsunit/compiler/optimized-for-in.js#newcode29
test/mjsunit/compiler/optimized-for-in.js:29: // Flags:
--no-speculative-concurrent-osr
On 2013/09/03 14:05:36, titzer wrote:
What's wrong here?
This is the only test where we use %OptimizeFunctionOnNextCall(fun,
"osr") to force OSR. But the loops are too short for the concurrent
thread to complete compilation and install, so with speculative
concurrent osr, we are not testing osr entry at all.
https://codereview.chromium.org/23710014/
--
--
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.
