I didn't looked into early. I have my small js file with more or less this pattern, it's used to check that closure indeed gets optimized, but it doesn't quite smell like a unittest. http://codereview.chromium.org/5753005/diff/9001/test/mjsunit/compiler/regress-closures-with-eval.js looks to cover pretty similar thing (access to argument, I know it's slightly different. I can throw access to local variable test as well.
Thanks again for your feedback! yours, anton. On Tue, Dec 14, 2010 at 3:39 AM, Florian Schneider <[email protected]> wrote: > 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
