Make closures optimizable by Crankshaft compiler.

Make closures optimizable by Crankshaft compiler.

Currently only closures which only read from the context are supported.

Committed: http://code.google.com/p/v8/source/detail?r=6340

[antonm, 2010-12-13 14:02:59]

Florian, Kasper and Kevin,

may you have a look?

[fschneider, 2010-12-13 19:00:18]

Here are my comments. 

Since I'm not so familiar with the scopes-code, Kevin should have a look, too.

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();
This checks if the function literal's scope has any heap allocated slots. So I
think this check can go away with your change.

http://codereview.chromium.org/5753005/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/5753005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:2520: virtual Representation
RequiredInputRepresentation(int index) const {
Since HLoadContextSlot does not have any input operands, you do not need to
implement RequiredInputRepresentation.

http://codereview.chromium.org/5753005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5753005/diff/1/src/hydrogen.cc#newcode2372
src/hydrogen.cc:2372: if (scope->num_heap_slots() > 0) BAILOUT("heap allocated
locals");
I think this bailout can go away, too.

http://codereview.chromium.org/5753005/diff/1/src/hydrogen.cc#newcode2934
src/hydrogen.cc:2934: variable->AsSlot()->type() == Slot::CONTEXT) {
Indentation.

http://codereview.chromium.org/5753005/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

http://codereview.chromium.org/5753005/diff/1/src/ia32/lithium-ia32.h#newcode...
src/ia32/lithium-ia32.h:1287: public:
Please also implement the PrintDataTo function.

http://codereview.chromium.org/5753005/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/5753005/diff/1/src/objects-inl.h#newcode2987
src/objects-inl.h:2987: void SharedFunctionInfo::set_code(Code* value,
WriteBarrierMode mode) {
I'm not sure why is this assert not needed anymore? (I'm also not sure why is
was needed before)

http://codereview.chromium.org/5753005/diff/1/test/cctest/cctest.status
File test/cctest/cctest.status (right):

http://codereview.chromium.org/5753005/diff/1/test/cctest/cctest.status#newco...
test/cctest/cctest.status:32: # The problem is code object can get different
optimizable flag in crankshaft after creation.
# The problem is that a code object can get a different
# optimizable flag in crankshaft after creation.

[antonm, 2010-12-13 22:41:04]

Florian,

thanks a lot for your comments.

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();
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/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/5753005/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:2520: virtual Representation
RequiredInputRepresentation(int index) const {
On 2010/12/13 19:00:18, fschneider wrote:
> Since HLoadContextSlot does not have any input operands, you do not need to
> implement RequiredInputRepresentation.

Done

http://codereview.chromium.org/5753005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5753005/diff/1/src/hydrogen.cc#newcode2372
src/hydrogen.cc:2372: if (scope->num_heap_slots() > 0) BAILOUT("heap allocated
locals");
On 2010/12/13 19:00:18, fschneider wrote:
> I think this bailout can go away, too.

I am not sure.  Apparently it makes us bail out of the function being compiled
needs to store some local variables in heap allocated slots, not when the
function accesses heap allocated slot of some other function.  That is:

function foo() {
  var heap_slot = 1;
  return function bar() {
    return heap_slot;
  }
}

This check bails out for foo, not for bar, if I understand it correctly.

http://codereview.chromium.org/5753005/diff/1/src/hydrogen.cc#newcode2934
src/hydrogen.cc:2934: variable->AsSlot()->type() == Slot::CONTEXT) {
On 2010/12/13 19:00:18, fschneider wrote:
> Indentation.

Done.

http://codereview.chromium.org/5753005/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

http://codereview.chromium.org/5753005/diff/1/src/ia32/lithium-ia32.h#newcode...
src/ia32/lithium-ia32.h:1287: public:
On 2010/12/13 19:00:18, fschneider wrote:
> Please also implement the PrintDataTo function.

Done.

http://codereview.chromium.org/5753005/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/5753005/diff/1/src/objects-inl.h#newcode2987
src/objects-inl.h:2987: void SharedFunctionInfo::set_code(Code* value,
WriteBarrierMode mode) {
On 2010/12/13 19:00:18, fschneider wrote:
> I'm not sure why is this assert not needed anymore? (I'm also not sure why is
> was needed before)

My reasoning: before only functions which allowed lazy compilation were
optimizable, now any function has a chance, hence the assert should go away. 
Does that sound reasonable?

http://codereview.chromium.org/5753005/diff/1/test/cctest/cctest.status
File test/cctest/cctest.status (right):

http://codereview.chromium.org/5753005/diff/1/test/cctest/cctest.status#newco...
test/cctest/cctest.status:32: # The problem is code object can get different
optimizable flag in crankshaft after creation.
On 2010/12/13 19:00:18, fschneider wrote:
> # The problem is that a code object can get a different
> # optimizable flag in crankshaft after creation.

Done.

[antonm, 2010-12-13 22:52:47]

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.

[fschneider, 2010-12-17 13:36:22]

LGTM from my side.

Kevin, do you have any more comments?

http://codereview.chromium.org/5753005/diff/17001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/5753005/diff/17001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:2528: SetFlag(kDependsOnCalls);
The flags are ok now since we don't compile stores to context slots. But once we
do, you need to add a new flag for stores and mark HLoadContextSlot depend on
it, so the GVN does the right thing.

http://codereview.chromium.org/5753005/diff/17001/test/mjsunit/closures.js
File test/mjsunit/closures.js (right):

http://codereview.chromium.org/5753005/diff/17001/test/mjsunit/closures.js#ne...
test/mjsunit/closures.js:29: for (var i = 0; i < 100000000; i++) {
100M seems a bit high (at least for debug mode) - Could this count be lower to
save time when running the tests and still hit the function with the optimizer?

[antonm, 2010-12-17 14:52:23]

Thanks a lot, Florian

http://codereview.chromium.org/5753005/diff/17001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/5753005/diff/17001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:2528: SetFlag(kDependsOnCalls);
On 2010/12/17 13:36:22, fschneider wrote:
> The flags are ok now since we don't compile stores to context slots. But once
we
> do, you need to add a new flag for stores and mark HLoadContextSlot depend on
> it, so the GVN does the right thing.

Thanks a lot for pointing this out.

http://codereview.chromium.org/5753005/diff/17001/test/mjsunit/closures.js
File test/mjsunit/closures.js (right):

http://codereview.chromium.org/5753005/diff/17001/test/mjsunit/closures.js#ne...
test/mjsunit/closures.js:29: for (var i = 0; i < 100000000; i++) {
On 2010/12/17 13:36:22, fschneider wrote:
> 100M seems a bit high (at least for debug mode) - Could this count be lower to
> save time when running the tests and still hit the function with the
optimizer? 

Found minimal value for my pretty beefy box.  Let's hope we'll trigger it on
various others as well

[Kevin Millikin (Chromium), 2010-12-20 12:05:44]

A few small comments.

I think when you look up a variable in an outer scope based on a serialized
scope info, and find it present in the context, you need to add it to the
variable map for that scope rather than allocating a fresh one.

I'm not entirely satisfied with patching up the scope chain after parsing and
before scope analysis.  It still seems cleaner that we pass a correct scope
chain into the parser for lazy parsing.  Maybe we should make that a goal for
long-term cleanup of this part of the code.

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen-instructions.c...
src/hydrogen-instructions.cc:1230: stream->Add("[%d->%d]]",
context_chain_length(), slot_index());
I think you have an extra "]" here.  I'm not wild about the ASCII arrow, because
it doesn't really signify anything.  "(%d,%d) seems fine to me.

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode2934
src/hydrogen.cc:2934: variable->AsSlot()->type() == Slot::CONTEXT) {
This predicate (->AsSlot() != NULL && ->AsSlot()->type == Slot::CONTEXT) should
probably be defined on the Variable class as something like "IsInContext()" or
"IsHeapAllocated()".

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode2965
src/hydrogen.cc:2965: BAILOUT("reference to non-stack-allocated/non-global
variable");
You should probably change this bailout string.  All that's left is
Slot::LOOKUP, right?

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode3463
src/hydrogen.cc:3463: if (var->AsSlot() != NULL && var->AsSlot()->type() ==
Slot::CONTEXT) {
Instead of an early bailout, it would be better if this code should be
structured like the code in VisitVariableProxy, explicitly handling the cases:

if (/* global */) ...
else if (/* stack-allocated */) ...
else /* the rest, bailout */

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode3472
src/hydrogen.cc:3472: } else {
Because here we don't want to have Slot::LOOKUP fall through the cracks.

http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1823: HLoadContextSlot* hinstr =
instr->hydrogen();
It's typical for the Lithium instructions to have accessors like slot_index and
context_chain_length that just forward to the hydrogen instruction.

http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1824: Comment(";;; Load context slot %d at %d",
There will automatically be a comment with the printed representation of the
instruction without having to emit it explicitly.

You should just have to override the print data function for LLoadContextSlot to
print the height and offset.

http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:274: hydrogen()->PrintDataTo(stream);
I wouldn't forward to the hydrogen instruction like this.  There is no guarantee
that the printed representation of a hydrogen instruction makes sense in a
Lithium context.

http://codereview.chromium.org/5753005/diff/27001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/objects.cc#newcode5381
src/objects.cc:5381: || code() == shared()->code());
For some reason we like to put || on the first line.  In any case, the second
line should be lined up under the "s" in shared() above it.

http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:139: && function->shared()->code() == code;
Why is this last check necessary?

http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:165: || !function->shared()->allows_lazy_compilation())
{
It makes more sense to move this check down below with the other checks on the
shared code (after SharedFunctionInfo* shared = function->shared()')

http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode133
src/scopes.cc:133: // At some point we might want to provide outer scopes to
Keep this comment near the ASSERT it mentions.

http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode241
src/scopes.cc:241: return new Variable(this, name, Variable::VAR, true,
Variable::ARGUMENTS);
We don't want to return a fresh variable every time we lookup the same symbol in
the same scope.  We lose the property that the same identifier always resolves
to the same variable in the same scope.

http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode246
src/scopes.cc:246: int index = scope_info_->ContextSlotIndex(*name, &mode);
We also want to assert (at least) that we do not find this name as a
stack-allocated variable in this scope.  If we do then something has gone wrong.

[antonm, 2010-12-20 20:39:24]

Thanks a lot for comments, Kevin.

As discussed on IM, I'm postponing hoisting of serialized scope info into parser
itself for this CL.

Would appreciated the next round.

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen-instructions.c...
src/hydrogen-instructions.cc:1230: stream->Add("[%d->%d]]",
context_chain_length(), slot_index());
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> I think you have an extra "]" here.  I'm not wild about the ASCII arrow,
because
> it doesn't really signify anything.  "(%d,%d) seems fine to me.

Done.

I used -> syntax to mimic C/C++ structure's field access

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode2934
src/hydrogen.cc:2934: variable->AsSlot()->type() == Slot::CONTEXT) {
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> This predicate (->AsSlot() != NULL && ->AsSlot()->type == Slot::CONTEXT)
should
> probably be defined on the Variable class as something like "IsInContext()" or
> "IsHeapAllocated()".

Done.

I named IsContextSlot as IMHO IsHeapAllocated rather leaks implementation detail
and IsInContext looks somewhat vague for me.  But if you prefer any other name,
just name it :)

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode2965
src/hydrogen.cc:2965: BAILOUT("reference to non-stack-allocated/non-global
variable");
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> You should probably change this bailout string.  All that's left is
> Slot::LOOKUP, right?

Done.

Please check if wording is fine.

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode3463
src/hydrogen.cc:3463: if (var->AsSlot() != NULL && var->AsSlot()->type() ==
Slot::CONTEXT) {
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> Instead of an early bailout, it would be better if this code should be
> structured like the code in VisitVariableProxy, explicitly handling the cases:
> 
> if (/* global */) ...
> else if (/* stack-allocated */) ...
> else /* the rest, bailout */

Done.

http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode3472
src/hydrogen.cc:3472: } else {
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> Because here we don't want to have Slot::LOOKUP fall through the cracks.

Done.

http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1823: HLoadContextSlot* hinstr =
instr->hydrogen();
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> It's typical for the Lithium instructions to have accessors like slot_index
and
> context_chain_length that just forward to the hydrogen instruction.

Done.

http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1824: Comment(";;; Load context slot %d at %d",
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> There will automatically be a comment with the printed representation of the
> instruction without having to emit it explicitly.
> 
> You should just have to override the print data function for LLoadContextSlot
to
> print the height and offset.

Done.

http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:274: hydrogen()->PrintDataTo(stream);
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> I wouldn't forward to the hydrogen instruction like this.  There is no
guarantee
> that the printed representation of a hydrogen instruction makes sense in a
> Lithium context.

Done.

http://codereview.chromium.org/5753005/diff/27001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/objects.cc#newcode5381
src/objects.cc:5381: || code() == shared()->code());
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> For some reason we like to put || on the first line.  In any case, the second
> line should be lined up under the "s" in shared() above it.

Done.

http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:139: && function->shared()->code() == code;
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> Why is this last check necessary?

Oops, remnants of previous attempts.  Removed.

http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:165: || !function->shared()->allows_lazy_compilation())
{
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> It makes more sense to move this check down below with the other checks on the
> shared code (after SharedFunctionInfo* shared = function->shared()')

Done.

http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode133
src/scopes.cc:133: // At some point we might want to provide outer scopes to
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> Keep this comment near the ASSERT it mentions.

Done.

http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode241
src/scopes.cc:241: return new Variable(this, name, Variable::VAR, true,
Variable::ARGUMENTS);
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> We don't want to return a fresh variable every time we lookup the same symbol
in
> the same scope.  We lose the property that the same identifier always resolves
> to the same variable in the same scope.

Thanks a lot for spotting this.

Thinking more about it: resolved scopes should be never innermost scopes and
thus apparently they never need to resolve arguments name as this way or enough
it must be always looked up by the innermost scope, correct?  Tests pass.

http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode246
src/scopes.cc:246: int index = scope_info_->ContextSlotIndex(*name, &mode);
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> We also want to assert (at least) that we do not find this name as a
> stack-allocated variable in this scope.  If we do then something has gone
wrong.

Done.

http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode246
src/scopes.cc:246: int index = scope_info_->ContextSlotIndex(*name, &mode);
On 2010/12/20 12:05:44, Kevin Millikin wrote:
> We also want to assert (at least) that we do not find this name as a
> stack-allocated variable in this scope.  If we do then something has gone
wrong.

Done.

[Kevin Millikin (Chromium), 2011-01-14 12:48:59]

I have a couple of small comments about where it's used in the compiler.

Other than that, LGTM.  Land it before Florian changes Lithium even more.

http://codereview.chromium.org/5753005/diff/40001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/5753005/diff/40001/src/compiler.cc#newcode665
src/compiler.cc:665: if (!MakeCrankshaftCode(&info)) {
The code here is confusing.  From my reading of the code:

MakeCrankshaftCode will check if the CompilationInfo allows optimizing, which
requires a closure.  Since we don't have one for BuildFunctionInfo, it's not
allowed from this call site.  Then it will mark the CompilationInfo as not to be
optimized (which it already is anyway) and then always compile with the baseline
compiler.

I think this does the same and is clearer:

if (V8::UseCrankshaft()) {
  if (!FullCodeGenerator::MakeCode(&info)) {
    return Handle...;
  }
  if (FLAG_optimize_closures ....

http://codereview.chromium.org/5753005/diff/40001/src/compiler.cc#newcode669
src/compiler.cc:669: && !AlwaysFullCompiler()
I'm not sure all these checks are necessary.  I would expect (and hope) that you
don't have to check AlwaysFullCompiler() here because we should never get into
the crankshaft backend for AlwaysFullCompiler().  I don't think you need to
require a non-trivial outer context (do you?).

http://codereview.chromium.org/5753005/diff/40001/src/compiler.cc#newcode674
src/compiler.cc:674: info.code()->set_optimizable(true);
Then, I'm not a fan of compiling with non-optimizable and then flipping a bit
after compilation to say that it actually is.  That is error-prone if we do
anything different for optimizable code during baseline compilation.  It would
be better to have these checks before compiling:

if (FLAG_optimize_closures && ...) {
  info->SetOptimizable()  // You'll have to implement this.
}

[antonm, 2011-01-14 15:03:47]

Kevin, thanks a lot for another round.

I've attempted to rework checks following your suggestions.  May you have
another look?  Changed files are ast.* and compiler.*

Currently I see some heisenbug, so I am not ready to commit it.  Plus I'll need
to do ports.

[Kevin Millikin (Chromium), 2011-01-14 15:15:05]

Yes, LGTM.