Never mind - I was misunderstanding that this change also enables the outer
function (foo in your example) to be optimized. But for that we need the
code to allocate a local context. I guess we want to enable this soon, too,
in a separate change.
So yes, it makes sense to have the check to bailout early in that case. It
would be nice to print out a message when --trace-bailout is enabled.
> function foo() {
> var heap_slot = 1;
> return function bar() {
> return heap_slot;
> }
> }
>
It would be nice to have a little unit-test that exercises the code (if we
don't already have test coverage).
Have you tried if we hit any functions in the early benchmark?
--Florian
Den 13. dec. 2010 23.52 skrev <[email protected]>:
>
> http://codereview.chromium.org/5753005/diff/1/src/compiler.cc
> File src/compiler.cc (right):
>
> http://codereview.chromium.org/5753005/diff/1/src/compiler.cc#newcode168
> src/compiler.cc:168: if (!info->AllowOptimize())
> info->DisableOptimization();
> BTW, if I understand meaning of Scope::num_heap_slots correctly, I was
> probably too quick to remove it---it allowed us to bail out earlier (not
> in ScopeSetup). What do you think?
>
>
> On 2010/12/13 22:41:04, antonm wrote:
>
>> On 2010/12/13 19:00:18, fschneider wrote:
>> > This checks if the function literal's scope has any heap allocated
>>
> slots. So I
>
>> > think this check can go away with your change.
>>
>
> I am not sure: CompilationInfo::AllowOptimize checks if crankshaft is
>>
> enabled
>
>> and if closure_ is not null. I removed Scope::AllowOptimize as per
>>
> your
>
>> suggestion on IM---thanks a lot for spotting this.
>>
>
> http://codereview.chromium.org/5753005/
>
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
