I have solved the problem with GC hitting while the inobject slack tracking
is
in progress.
Comments addressed.
Still need to add a test for multiple contexts.
http://codereview.chromium.org/3329019/diff/1/2
File src/arm/builtins-arm.cc (right):
http://codereview.chromium.org/3329019/diff/1/2#newcode534
src/arm/builtins-arm.cc:534: // Use r7 for holding undefined which is
used in several places below.
On 2010/09/10 12:07:13, Mads Ager wrote:
No reason to load undefined into r7 here anymore I think. I think we
are only
using r7 in one place now. So just load filler into r7 just before
loop and
undefined into r7 just before the other loop. That way you don't have
to save
and restore r7 when finalizing the instance size.
Done.
http://codereview.chromium.org/3329019/diff/1/2#newcode573
src/arm/builtins-arm.cc:573: // rcx: shared function info
On 2010/09/10 12:07:13, Mads Ager wrote:
rcx -> r3
Maybe just remove the comment since you just loaded it. Seems obvious.
Done.
http://codereview.chromium.org/3329019/diff/1/2#newcode578
src/arm/builtins-arm.cc:578: __ strb(r4, constructor_count);
On 2010/09/10 12:07:13, Mads Ager wrote:
Doesn't this means that the count will wrap around and you will redo
the map
tree traversal again later (at which point you cannot gain anything).
It would
be nice to avoid that.
Edit: It doesn't mean that it is done again - I see that you change
the stub
when calling finalize - could you add a comment about that here?
Done.
http://codereview.chromium.org/3329019/diff/1/2#newcode581
src/arm/builtins-arm.cc:581: __ push(r1);
There is no Pop macro as far as I was able to see.
On 2010/09/10 12:07:13, Mads Ager wrote:
Using Push and Pop macros you should be able to use strm and ldrm for
r2 and r1
here.
http://codereview.chromium.org/3329019/diff/1/2#newcode618
src/arm/builtins-arm.cc:618: // Fill all the in-object properties with
undefined.
On 2010/09/10 12:07:13, Mads Ager wrote:
with undefined -> with one-word fillers.
Done.
http://codereview.chromium.org/3329019/diff/1/5
File src/heap.cc (right):
http://codereview.chromium.org/3329019/diff/1/5#newcode2698
src/heap.cc:2698: Object* filler;
Added a comment.
On 2010/09/10 12:07:13, Mads Ager wrote:
Do we need the conditional here?
Is this because of internal fields from API functions? If that is the
case it
requires a comment! :)
http://codereview.chromium.org/3329019/diff/1/7
File src/ia32/assembler-ia32.h (right):
http://codereview.chromium.org/3329019/diff/1/7#newcode598
src/ia32/assembler-ia32.h:598: void dec_b(const Operand& dst);
The disasssembler does not handle 'dec' properly, let me fix that in a
separate patch.
On 2010/09/10 12:07:13, Mads Ager wrote:
Do you need to update the disassembler as well or does it already
handle this?
http://codereview.chromium.org/3329019/diff/1/8
File src/ia32/builtins-ia32.cc (right):
http://codereview.chromium.org/3329019/diff/1/8#newcode191
src/ia32/builtins-ia32.cc:191: // To allow for truncation
On 2010/09/10 15:28:50, antonm wrote:
nit: missing dot at the end of the comment.
Done.
http://codereview.chromium.org/3329019/diff/1/8#newcode193
src/ia32/builtins-ia32.cc:193: __ mov(edx,
Factory::one_pointer_filler_map());
We never allocate API objects with this code. I have added an assert to
the corresponding C++ code in InitializeJSObjectFromMap (which will be
called the first time the constructor is called).
On 2010/09/10 15:28:50, antonm wrote:
if we allocate API objects with the code, it might be a problem due to
internal
fields usage (see also Mads' comment in C++ part). Overall this
difference in
the logic might be dangerous
http://codereview.chromium.org/3329019/diff/1/9
File src/objects-inl.h (right):
http://codereview.chromium.org/3329019/diff/1/9#newcode1346
src/objects-inl.h:1346: void JSObject::InitializeBody(int object_size,
Object* value) {
On 2010/09/10 12:07:13, Mads Ager wrote:
Please add an assert that the value is not in new space. There is no
write
barrier in this code.
Done.
http://codereview.chromium.org/3329019/diff/1/10
File src/objects.cc (right):
http://codereview.chromium.org/3329019/diff/1/10#newcode3294
src/objects.cc:3294:
map->set_visitor_id(StaticVisitorBase::GetVisitorId(map));
This is needed because visitor id might depend on the instance size
which we just have changed.
Added the comment (the function moved).
On 2010/09/10 15:28:50, antonm wrote:
why is this needed? could you add a comment?
http://codereview.chromium.org/3329019/diff/1/10#newcode3298
src/objects.cc:3298: if (descriptors->GetType(i) == MAP_TRANSITION ||
On 2010/09/10 12:07:13, Mads Ager wrote:
Don't we have an IsTransition method? We should have so this will not
break if
we add a new transition type.
Removed (doing stackless traversal now).
http://codereview.chromium.org/3329019/diff/1/10#newcode5417
src/objects.cc:5417: if (unused_inobject_properties != 0) {
It is intended. The IsInobjectSlackTrackingStarted method is now called
differently (more clearly).
On 2010/09/10 15:28:50, antonm wrote:
if unused_inobject_properties == 0, how would this code behave?
apparently
you'll go into started, but not in progress state, is it intended?
http://codereview.chromium.org/3329019/diff/1/10#newcode5434
src/objects.cc:5434:
set_inobject_slack_estimation(unused_inobject_properties);
Removed the method
On 2010/09/14 13:24:01, Vitaly wrote:
If unused_inobject_properties is 0, then this half-cancels the
tracking
(IsInobjectSlackTrackingInProgress() becomes false). Is this a
problem? If not,
it's still too subtle and requires a comment.
http://codereview.chromium.org/3329019/diff/1/10#newcode5447
src/objects.cc:5447:
set_construct_stub(Builtins::builtin(Builtins::JSConstructStubGeneric));
On 2010/09/10 15:28:50, antonm wrote:
do we need an assert that current constuct_stub is
JSConstructStubCountdown?
Done.
http://codereview.chromium.org/3329019/diff/1/10#newcode5454
src/objects.cc:5454: // Make the counter runs out the next time.
Removed the method.
On 2010/09/14 13:24:01, Vitaly wrote:
Why do we have to postpone it? Can we call Complete...Tracking() here?
http://codereview.chromium.org/3329019/diff/1/10#newcode5454
src/objects.cc:5454: // Make the counter runs out the next time.
Removed the method
On 2010/09/10 15:28:50, antonm wrote:
nit: runs -> run
http://codereview.chromium.org/3329019/diff/1/11
File src/objects.h (right):
http://codereview.chromium.org/3329019/diff/1/11#newcode3239
src/objects.h:3239: // via map transitions. Should be applied only to a
map at the top of the
removed the method.
On 2010/09/10 15:28:50, antonm wrote:
nit: top -> root (of the transition tree)
http://codereview.chromium.org/3329019/diff/1/11#newcode3246
src/objects.h:3246: // The size limit for a map transition tree that can
be safely processed
Many thanks for the code. Implemented stackless traversal, have no more
fear.
On 2010/09/14 13:24:01, Vitaly wrote:
If we have a "class" with 51 functions, does this limit mean we'll
always be too
generous when allocating the instances? If yes, this could be bad for
memory
usage. I understand that this limit should not affect the normal usage
of
putting the functions on the prototype, but some OO frameworks put the
functions
on the instance.
Please have a look at http://codereview.chromium.org/3385002 where I
attempt to
do a stackless traversal of the map tree.
http://codereview.chromium.org/3329019/diff/1/11#newcode3248
src/objects.h:3248: static const int kMaxShrinkableTreeSize = 50;
doing stackless traversal now
On 2010/09/10 15:28:50, antonm wrote:
just curious: do we have any data on tree size distribution?
http://codereview.chromium.org/3329019/diff/1/11#newcode3248
src/objects.h:3248: static const int kMaxShrinkableTreeSize = 50;
doing stackless traversal now.
On 2010/09/14 13:24:01, Vitaly wrote:
kMaxShrinkable*Transition*TreeSize?
http://codereview.chromium.org/3329019/diff/1/11#newcode3457
src/objects.h:3457: // Inobject slack tracking:
Will experiment.
On 2010/09/10 12:07:13, Mads Ager wrote:
I wonder if we really have to keep track of this as we build the map
transitions? Seems a little heavy to have all these counters going and
having to
update them when we introduce a new map transition.
Is the traversal of the map tree fast enough that we could do one pass
to find
the new number and one pass to set it (bailing out during the
iteration if we
see too many maps)?
Maybe it is too expensive, but it seems much simpler to me.
http://codereview.chromium.org/3329019/diff/1/11#newcode3457
src/objects.h:3457: // Inobject slack tracking:
Removed on-the-fly transition tracking in favour of traversing the
transition tree at the tracking phase completion.
On 2010/09/10 12:07:13, Mads Ager wrote:
I wonder if we really have to keep track of this as we build the map
transitions? Seems a little heavy to have all these counters going and
having to
update them when we introduce a new map transition.
Is the traversal of the map tree fast enough that we could do one pass
to find
the new number and one pass to set it (bailing out during the
iteration if we
see too many maps)?
Maybe it is too expensive, but it seems much simpler to me.
http://codereview.chromium.org/3329019/diff/1/11#newcode3463
src/objects.h:3463: // To reclaims unused inobject space we:
On 2010/09/14 13:24:01, Vitaly wrote:
reclaims -> reclaim.
Done.
http://codereview.chromium.org/3329019/diff/1/11#newcode3488
src/objects.h:3488: inline bool IsInobjectSlackTrackingInProgress();
On 2010/09/14 13:24:01, Vitaly wrote:
Please describe the full state machine here.
Done. I think.
http://codereview.chromium.org/3329019/diff/1/11#newcode3493
src/objects.h:3493: inline void InitializeInobjectSlackTracking();
On 2010/09/10 12:07:13, Mads Ager wrote:
Please add comments explaining each of these.
Done.
http://codereview.chromium.org/3329019/diff/1/11#newcode3713
src/objects.h:3713: #warning Unknown byte ordering
The only other place that checks this uses #warning. But you are right,
this must be an error.
On 2010/09/10 15:28:50, antonm wrote:
#error?
http://codereview.chromium.org/3329019/diff/1/11#newcode3721
src/objects.h:3721: static const int kSize =
kInobjectSlackTrackingOffset + 4;
On 2010/09/10 12:07:13, Mads Ager wrote:
Isn't kIntSize the right thing here?
Removed.
http://codereview.chromium.org/3329019/diff/1/12
File src/runtime.cc (right):
http://codereview.chromium.org/3329019/diff/1/12#newcode6763
src/runtime.cc:6763: if (first_allocation &&
!shared->IsInobjectSlackTrackingInProgress()) {
NewJSObject call could change the state. Specifically, ...InProgress
will be false if Start... method skipped over the tracking phase. It is
now a bit more clear with new method names.
On 2010/09/10 15:28:50, antonm wrote:
isn't it true that IsInobjectSlackTrackingInProgress => Started, hence
you don't
need && at all?
http://codereview.chromium.org/3329019/diff/1/12#newcode6781
src/runtime.cc:6781: // If the constructor isn't a proper function we
throw a type error.
This gets called with non-function argument from natives fuzzer. Not
sure what is the right thing to do.
Returning undefined from the main branch.
On 2010/09/10 12:07:13, Mads Ager wrote:
This seems a bit strange to me. When does this happen and why don't
you just
return undefined in that case? Do you ever use the result of this
call?
http://codereview.chromium.org/3329019/show
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
