Here you go Hannes, thx for the comments, they prompted a few bigger
changes.
--Michael
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/flag-definitions.h#newcode213
src/flag-definitions.h:213: DEFINE_bool(allocation_site_pretenuring,
true,
On 2013/11/21 20:52:24, Hannes Payer wrote:
Should be set to false.
Done.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/heap-inl.h
File src/heap-inl.h (right):
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/heap-inl.h#newcode495
src/heap-inl.h:495: // Update state
On 2013/11/21 20:52:24, Hannes Payer wrote:
Remove this comment.
Done.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/heap-inl.h#newcode502
src/heap-inl.h:502:
object->GetIsolate()->heap()->allocation_mementos_found_++;
On 2013/11/21 20:52:24, Hannes Payer wrote:
Do we still need this variable? We could just ask the allocation
sites.
Done. Facilitated by moving the allocation site feedback digestion to
the gc epilogue.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/heap.cc
File src/heap.cc (right):
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/heap.cc#newcode1557
src/heap.cc:1557: if (!(casted->DecisionMade())) {
On 2013/11/21 20:52:24, Hannes Payer wrote:
See the other comment about not enough allocation mementos created -
here I
would stay in undecided mode.
I've simplified and encapsulated this code in a way that also respects a
minimum number of mementos found.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/heap.cc#newcode1566
src/heap.cc:1566: }
On 2013/11/21 20:52:24, Hannes Payer wrote:
Right now we do not support switching state which is OK. But let's add
a TODO
here.
Done.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/heap.cc#newcode4403
src/heap.cc:4403: allocation_site->IncrementMementoCreateCount();
On 2013/11/21 20:52:24, Hannes Payer wrote:
I am wondering if this counter is not only interesting for pretenuring
but also
for other clients of allocation mementos.
Maybe so, but not yet. I like the idea of keeping all this
infrastructure behind the pretenuring flag for now, at least until there
is another client or we can get rid of the flag.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/heap.h
File src/heap.h (right):
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/heap.h#newcode1460
src/heap.h:1460: // some extra work.
On 2013/11/21 20:52:24, Hannes Payer wrote:
Ambiguous comment, can we be more precise here.
Thx, I renamed the method to UpdateAllocationSiteFeedback to clarify
meaning. I'll keep the comment there because it's the style of heap.h,
but modify it a bit.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/heap.h#newcode1464
src/heap.h:1464: void EnsureFromSpaceIsCommitted();
On 2013/11/21 20:52:24, Hannes Payer wrote:
I did not find the reason why this method has to be public.
Me neither! fixed.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/hydrogen.cc#newcode2256
src/hydrogen.cc:2256: // Increment create count
On 2013/11/21 20:52:24, Hannes Payer wrote:
Beautify comment.
Done.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/hydrogen.cc#newcode2268
src/hydrogen.cc:2268: store->SkipWriteBarrier(); // I am writing a smi
On 2013/11/21 20:52:24, Hannes Payer wrote:
Move comment in extra line and make it nice.
Done.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/objects-inl.h
File src/objects-inl.h (right):
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/objects-inl.h#newcode1400
src/objects-inl.h:1400: double ratio = create_count > 0
On 2013/11/21 20:52:24, Hannes Payer wrote:
We have to guard against the case where we just started emitting
mementos before
a gc happend. In that state we cannot make a sane pretenuring
decision.
Moreover, we do not want to pretenure non-interesting sites, i.e.
sites that
just allocated one object.
Let us add a constant. If the create count is smaller than this
constant we
return false. A good value for that constant is hard to find, we have
to look
into benchmarks for that but let us start with something reasonable
like e.g.
kPretenureMinimumCreated = 100
Good advice. Done.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/objects-inl.h#newcode1406
src/objects-inl.h:1406: return ratio >= 0.60;
On 2013/11/21 20:52:24, Hannes Payer wrote:
This should be a constant in allocation site called e.g.
kPretenureRatio.
Done. Awkwardly, it must be initialized in objects.cc because of the
rules around floating point constants.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/objects.h
File src/objects.h (right):
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/objects.h#newcode8110
src/objects.h:8110: static const int kZombie = 3;
On 2013/11/21 20:52:24, Hannes Payer wrote:
Right now the state diagram (should) looks like that:
0 -> 1
0 -> 2
1 -> 3
2 -> 3
The 0 -> 3 transition is missing see my comments.
A follow up CL should add the 1 -> 2 and 2 -> 1 transitions.
Thx. I made these 4 state values into an enum to indicate grouping and
because of a compiler issue.
The new TODO in heap.cc talks about revisiting the decision for the 1->2
and 2->1 cases.
Finally, I think the zombie state is okay. But it's a strange concept so
I added a long comment right before IsZombie(), lets chat about that.
https://chromiumcodereview.appspot.com/40063002/diff/350001/src/objects.h#newcode8131
src/objects.h:8131:
On 2013/11/21 20:52:24, Hannes Payer wrote:
Can we have newlines between the methods?
Done.
https://chromiumcodereview.appspot.com/40063002/
--
--
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/groups/opt_out.
