Even though I have a bunch of comments, this is pretty close.
http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.cc File src/code-stubs.cc (right): http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.cc#newcode105 src/code-stubs.cc:105: if (use_special_cache || !FindCodeInCache(&code)) { I think this is a little clearer if it is something like: if (use_special_cache) { if (FindCodeInSpecialCache(&code)) return Handle<Code>(code); } else { if (FindCodeInCache(&code)) return Handle<Code>(code); } or if you wanted to be a bit twisted: if (use_special_cache ? FindCodeInSpecialCache(&code) : FindCodeInCache(&code)) { return Handle<Code>(code); } http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.cc#newcode176 src/code-stubs.cc:176: Isolate* isolate = Isolate::Current(); If you want to avoid TLS to fetch the isolate, you could pass in the one you have from GetCode or else use new_object->isolate(). http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.h File src/code-stubs.h (right): http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.h#newcode447 src/code-stubs.h:447: static void GenerateKnownObjects(MacroAssembler* masm, Map* map); Put this down in the private section with the other Generate... functions. Make it non-static. Do not pass map (== known_map_) as an argument, but instead use known_map_ in the generate function. http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.h#newcode449 src/code-stubs.h:449: void set_known_map(Map* map) { known_map_ = map; } Make map a Handle<Map>. There's a risk that a GC occurs between set_known_map(map) and Generate(). http://codereview.chromium.org/8520006/diff/17001/src/code-stubs.h#newcode476 src/code-stubs.h:476: Map* known_map_; Handle<Map>. http://codereview.chromium.org/8520006/diff/17001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/8520006/diff/17001/src/hydrogen.cc#newcode6000 src/hydrogen.cc:6000: case Token::EQ_STRICT: { I guess if EQ_STRICT, then we'll never have a compare map (because we don't go to KNOWN_OBJECTS in that case). I wonder if we should? http://codereview.chromium.org/8520006/diff/17001/src/ia32/code-stubs-ia32.cc File src/ia32/code-stubs-ia32.cc (right): http://codereview.chromium.org/8520006/diff/17001/src/ia32/code-stubs-ia32.cc#newcode6521 src/ia32/code-stubs-ia32.cc:6521: __ bind(&miss); All the code from here and below looks like it could be generated by a call to GenerateMiss(masm) (if this function were non-static). http://codereview.chromium.org/8520006/diff/17001/src/ia32/code-stubs-ia32.cc#newcode6524 src/ia32/code-stubs-ia32.cc:6524: __ pop(ecx); And come to think of it, I don't know why we preserve the incoming arguments under the return address in the existing code. Instead, it would be a bit more straightforward to save them above and save a pair of push/pops to do: { FrameScope scope(masm, StackFrame::INTERNAL); __ push(edx); // Preserve edx and eax. __ push(eax); __ push(edx); // And also use them as the arguments. __ push(eax); __ push(Immediate(Smi::FromInt(Token::EQ))); __ CallExternalReference(runtime, 3); __ pop(eax); __ pop(edx); } // Compute the entry point of the rewritten stub. __ lea(edi, FieldOperand(eax, Code::kHeaderSize)); // Do a tail call to the rewritten stub. __ jmp(edi); http://codereview.chromium.org/8520006/diff/17001/src/ia32/ic-ia32.cc File src/ia32/ic-ia32.cc (right): http://codereview.chromium.org/8520006/diff/17001/src/ia32/ic-ia32.cc#newcode1630 src/ia32/ic-ia32.cc:1630: rewritten = stub.GetCode(true); Nicer to have a virtual "UseSpecialCache" function you can override for just this stub, instead of the parameter (with a default, and a boolean). http://codereview.chromium.org/8520006/diff/17001/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/8520006/diff/17001/src/ic.cc#newcode2336 src/ic.cc:2336: if (state == UNINITIALIZED && x->IsSmi() && y->IsSmi()) { I don't like this function. It's really hard to figure out what transitions will happen. It seems to be programmed after listing the possible output types with some exceptions up front, but that's not the way that most people think about state transitions. What about rewriting it to something more structured like: switch (state) { case UNINITIALIZED: if (x->IsSmi() && y->IsSmi()) return SMIS; if (x->IsNumber() && y->IsNumber()) return HEAP_NUMBERS; if (!Token::IsEqualityOp(op_)) return GENERIC; if (x->IsSymbol() && y->IsSymbol()) return SYMBOLS; if (x->IsString() && y->IsString()) return STRINGS; if (x->IsJSObject() && y->IsJSObject()) { // Your new stuff here. } return GENERIC; case SMIS: return has_inlined_smi_code && x->IsNumber() && y->IsNumber() ? HEAP_NUMBERS : GENERIC; case SYMBOLS: ASSERT(Token::IsEqualityOp(op_)); return x->IsString() && y->IsString() ? STRINGS : GENERIC; case HEAP_NUMBERS: case STRINGS: case OBJECTS: case KNOWN_OBJECTS: case GENERIC: return GENERIC; } http://codereview.chromium.org/8520006/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
