Another upload fixed the problem. On Fri, Jul 2, 2010 at 12:36 PM, Mads Sig Ager <[email protected]> wrote:
> Sergey, > > I get consistent '500 Server Error' messages when attempting to access > this codereview (but I can access other code reviews just fine). Could > you attempt another upload and if that does not work create a new one? > > Thanks, -- Mads > > On Thu, Jul 1, 2010 at 9:21 PM, <[email protected]> wrote: > > > > http://codereview.chromium.org/2801018/diff/54003/66007 > > File src/ia32/stub-cache-ia32.cc (right): > > > > http://codereview.chromium.org/2801018/diff/54003/66007#newcode104 > > src/ia32/stub-cache-ia32.cc:104: // Helper function used to check that > > the dictionary doesn't contain the prop. > > On 2010/07/01 10:11:29, Mads Ager wrote: > >> > >> prop -> property. > > > > Done. > > > > http://codereview.chromium.org/2801018/diff/54003/66007#newcode172 > > src/ia32/stub-cache-ia32.cc:172: // Having undefined at this place means > > the name is not contained. > > On 2010/07/01 10:11:29, Mads Ager wrote: > >> > >> Please expand this comment to state that the undefined check takes > > > > care of both > >> > >> present and deleted elements because deleted elements use null as the > > > > name. > > > > I woudn't say this is the place which takes care of deleted elements. > > Instead I added a comment above describing why this method is correct > > and why deleted values don't break it. > > > > http://codereview.chromium.org/2801018/diff/54003/66007#newcode177 > > src/ia32/stub-cache-ia32.cc:177: if (entity_name.is(properties)) { > > On 2010/07/01 10:11:29, Mads Ager wrote: > >> > >> Could you use extra.is(no_reg) for consistency with the condition for > > > > pushing > >> > >> the receiver? > > > > Done. > > > > http://codereview.chromium.org/2801018/diff/54003/66007#newcode187 > > src/ia32/stub-cache-ia32.cc:187: if (entity_name.is(properties)) { > > On 2010/07/01 10:11:29, Mads Ager wrote: > >> > >> Could you use extra.is(no_reg) here as well? > > > > Done. > > > > http://codereview.chromium.org/2801018/diff/54003/66007#newcode828 > > src/ia32/stub-cache-ia32.cc:828: Register > > StubCompiler::CheckPrototypes(JSObject* object, > > On 2010/07/01 10:11:29, Mads Ager wrote: > >> > >> Comments should be added to CheckPrototypes to state what it does. > > > > Folding > >> > >> CheckMaps into CheckPrototypes makes sense, but we should document > > > > what > >> > >> CheckPrototypes does. > > > >> Please document register use, push_at_depth, and what can be expected > > > > as a > >> > >> result of calling CheckPrototypes. Starting from the CheckMaps comment > > > > that has > >> > >> now been removed would make sense. > > > > Added comments to stub-cache.h. > > > > http://codereview.chromium.org/2801018/diff/54003/66007#newcode839 > > src/ia32/stub-cache-ia32.cc:839: // registers. > > On 2010/07/01 10:11:29, Mads Ager wrote: > >> > >> other registers -> holder and object registers. > > > >> You do not check extra and you probably don't have to? > > > > Done. > > > > http://codereview.chromium.org/2801018/diff/54003/66007#newcode930 > > src/ia32/stub-cache-ia32.cc:930: // Check that the object layout has not > > changed (if it's a fast properties > > On 2010/07/01 10:11:29, Mads Ager wrote: > >> > >> Could you go back to the original "Check the holder map." comment > > > > there. I don't > >> > >> find the comment that you have now useful. The map check implies a lot > > > > of things > >> > >> and not just for properties of type CONSTANT_FUNCTION. It does not > > > > imply that > >> > >> the object is the same. > > > > Done. > > > > http://codereview.chromium.org/2801018/diff/54003/66007#newcode940 > > src/ia32/stub-cache-ia32.cc:940: // Perform security check for access to > > the global object and return > > On 2010/07/01 10:11:29, Mads Ager wrote: > >> > >> Remove the 'return the holder register part' since you do not do this > > > > yet. > > > > Done. > > > > http://codereview.chromium.org/2801018/diff/54003/66007#newcode948 > > src/ia32/stub-cache-ia32.cc:948: current = object; > > On 2010/07/01 10:11:29, Mads Ager wrote: > >> > >> Please reinstate the comment about having to check for global property > > > > cell > >> > >> equality if we have skipped any global objects. > > > > Done. > > > >> I still think it would make sense to split this into a couple of > > > > helper methods > >> > >> to make this code easier to read. This method could be something like: > > > >> CheckPrototypes: > >> if receiver is slow case: > >> NegativeDictionaryLookup on receiver > >> object = receiver->GetPrototype() > >> CheckMaps from object to holder > >> CheckGlobalPropertyCells > > > >> I would find that much easier to read as the user of this method. Then > > > > you can > >> > >> dive into the details of the individual parts if you want to. > > > > I extracted methods GenerateCheckPropertyHolder and > > GenerateCheckPropertyCells, but I I left the main loop here. I still > > dislike the CheckMaps method (one more reason I dislike it is the > > push_at_depth argument). Would be cool to move a piece of the loop body > > in to a method but I don't see how to do it keeping its optimizations. > > I'l think for a while. > > > > http://codereview.chromium.org/2801018/diff/54003/66008 > > File src/ic-inl.h (right): > > > > http://codereview.chromium.org/2801018/diff/54003/66008#newcode99 > > src/ic-inl.h:99: // the same prototype (or a prototypes with the same > > map) and not having > > On 2010/07/01 10:11:29, Mads Ager wrote: > >> > >> or a prototypes -> or a prototype > > > > Done. > > > > http://codereview.chromium.org/2801018/show > > > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
