I like this change. Mostly nits, only one major comment in the
JSConstructStub.
Note that I only looked at the ia32 port, but my comments probably apply to
all
ports.
https://codereview.chromium.org/283383006/diff/60001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):
https://codereview.chromium.org/283383006/diff/60001/src/ia32/builtins-ia32.cc#newcode161
src/ia32/builtins-ia32.cc:161: // esi: slack tracking counter
nit: At this point esi does not yet contain the counter, it is loaded
below. Let's drop this comment.
https://codereview.chromium.org/283383006/diff/60001/src/ia32/builtins-ia32.cc#newcode242
src/ia32/builtins-ia32.cc:242: __ jmp(&done_field_initialization);
This will skip initialization of the memento even though we allocated
space for it. This will probably throw off the heap verifier.
Talking to Michael Stanton, he agreed that the safest path forward would
be to not allocate a Memento while slack-tracking is in progress.
https://codereview.chromium.org/283383006/diff/60001/src/objects.h
File src/objects.h (left):
https://codereview.chromium.org/283383006/diff/60001/src/objects.h#oldcode7008
src/objects.h:7008: inline void BeforeVisitingPointers();
The only reason we needed this callback was for detaching the initial
map from the SharedFunctionInfo. Since we ne longer do that I propose to
remove this callback, this in turn makes the
BeforeVisitingSharedFunctionInfo function in the static visitors
obsolete as well.
I am totally fine to do that in a follow-up CL, because this one is
already complex enough.
https://codereview.chromium.org/283383006/diff/60001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode5967
src/objects.h:5967: class DoneInobjectSlackTracking: public
BitField<bool, 28, 1> {};
nit: Alignment looks off.
https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode5970
src/objects.h:5970: class ConstructionCount: public
BitField<unsigned, 29, 3> {};
nit: With three bits it should be fine to use plain "int" here.
https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode7511
src/objects.h:7511: static const unsigned kGenerousAllocationCount =
nit: Likewise, plain "int" should do it here.
https://codereview.chromium.org/283383006/diff/60001/src/objects.h#newcode7512
src/objects.h:7512: (1 << Map::ConstructionCount::kSize) - 1;
nit: Just use "Map::ConstructionCount::kMax" here.
https://codereview.chromium.org/283383006/diff/60001/test/cctest/test-mementos.cc
File test/cctest/test-mementos.cc (right):
https://codereview.chromium.org/283383006/diff/60001/test/cctest/test-mementos.cc#newcode94
test/cctest/test-mementos.cc:94: unsigned int call_count =
JSFunction::kGenerousAllocationCount + 2;
nit: Likewise, plain "int" should do it here.
https://codereview.chromium.org/283383006/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
