Guys, may you have another look?
New features: 1) flag to control optimization; 2) scopes refactoring, now all the ugliness belongs to scopes.cc; 3) first step into sharing of the code: context is resolved dynamically. Alas, no proper sharing due to the fact that we have no way to store a code per shared function and context. Kasper (p.c.) says it might be a general problem, I'll try to address it and then reuse the infrastructure for closures. tia and yours, anton. On Tue, Dec 14, 2010 at 5:22 PM, Anton Muhin <[email protected]> wrote: > 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
