It's a little confusing (probably because of the original code). The biggest thing is that I think "InvalidateFrameSlotAt" should just handle any kind of frame element.
I think that makes it cleaner. We can eventually replace direct assignments of invalid elements with calls to this function (eg, when we Drop or Forget elements, we have to do essentially all this work anyway). And it gives us a single point to hang "becoming invalid" behavior in the future. http://codereview.chromium.org/50012/diff/7/12 File src/virtual-frame-arm.h (right): http://codereview.chromium.org/50012/diff/7/12#newcode450 Line 450: // Presently does nothing on ARM. This comment should just be identical to the IA32 one (mutatis mutandis). "Does nothing on ARM" has to be maintained (and isn't correct anyway---calling it does something by terminating the process in release and debug builds). http://codereview.chromium.org/50012/diff/7/8 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/50012/diff/7/8#newcode501 Line 501: // Before changing an element which is copied, adjust so that the Change this comment to correspond to the changes to the code. http://codereview.chromium.org/50012/diff/7/8#newcode506 Line 506: FrameElement VirtualFrame::InvalidateFrameSlot(int index) { To match the names of everything else, this should be the slightly longer "InvalidateFrameSlotAt". http://codereview.chromium.org/50012/diff/7/8#newcode510 Line 510: // Set it to invalid, and free the register. This comment is confusing. It mentions freeing a register (the original may not even be a register), but I don't see any code to do that. http://codereview.chromium.org/50012/diff/7/8#newcode512 Line 512: elements_[index].clear_copied(); Invalid elements (in fact, all new elements) are already created with their copy flags cleared, so this line isn't needed. http://codereview.chromium.org/50012/diff/7/8#newcode523 Line 523: Unuse(original.reg()); I think it's more clear to have the code to set elements_[index] to invalid here, close to the unuse of the register (even if it means duplicating that line for the other case). It's too hard to track related state changes that are separated too much in the code. http://codereview.chromium.org/50012/diff/7/8#newcode541 Line 541: FrameElement::SyncField::decode(elements_[i].type())); Even though VirtualFrame is a friend of FrameElement, it shouldn't depend on the representation of FrameElements (that it's a bit field). Go through the accessors (or add a new one if you need). http://codereview.chromium.org/50012/diff/7/8#newcode544 Line 544: FrameElement copy = CopyElementAt(i); This has a slight drawback of marking the new backing store as copied even if it's not. http://codereview.chromium.org/50012/diff/7/8#newcode553 Line 553: return copy; It seems like there's a slicker way to do this. * Hold off marking the new backing store as copied until the loop looking for other copies finds one (sometimes we want a copy as the return value, but other times we will just ignore it anyway). * For all the copies, can't we just set their data_.index_ member to be the new backing store (that would automatically preserve the sync flag)? * Instead of returning a copy of the new backing store or invalid if the original wasn't copied, can't we just return the index of the new backing store or kIllegalIndex, and then decide whether to copy it in the caller (where we know if we want a copy)? http://codereview.chromium.org/50012/diff/7/8#newcode620 Line 620: elements_[index] = FrameElement::InvalidElement(); If you made InvalidateFrameSlot work on copies and constants, you wouldn't need this line? http://codereview.chromium.org/50012/diff/7/8#newcode632 Line 632: // If the stored-to slot may be copied, adjust to preserve the Adjust this comment to reflect the new code. http://codereview.chromium.org/50012/diff/7/8#newcode634 Line 634: if (original.is_register() || original.is_memory()) { It's not obvious why InvalidateFrameSlot won't do the right thing for non-register, non-memory frame elements. http://codereview.chromium.org/50012/diff/7/10 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/50012/diff/7/10#newcode436 Line 436: // Helper function to implement the copy-on-write semantics of an This comment needs to be updated: the main point is that it invalidates a frame element (not necessarily just before writing to it anymore), unusing registers and preserving the copy invariants as needed. http://codereview.chromium.org/50012/diff/7/9 File src/virtual-frame.cc (right): http://codereview.chromium.org/50012/diff/7/9#newcode380 Line 380: // semantics of copied elements. Change this comment, it's no longer correct. http://codereview.chromium.org/50012/diff/7/9#newcode381 Line 381: if (original.is_register() || original.is_memory()) { It's not obvious why InvalidateFrameSlot won't do the right thing for non-register, non-memory frame elements, or is it just that it's unnecessary (the comment should explain). http://codereview.chromium.org/50012 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
