Hi Hannes, thanks for the review, I've updated per your comments. Three
things
to address:
1) There is new code, giving support to new Array() call sites.
2) ports are not done.
3) I'm doing a perf investigation on kraken, might result in some changes
but
they should be local.
https://codereview.chromium.org/15094018/diff/17001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/15094018/diff/17001/src/code-stubs-hydrogen.cc#newcode320
src/code-stubs-hydrogen.cc:320: AddInstruction(new(zone)
HLoadKeyed(literals,
On 2013/06/25 16:35:07, Hannes Payer wrote:
literals is just used there, you could just use the original code.
Done.
https://codereview.chromium.org/15094018/diff/17001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/15094018/diff/17001/src/flag-definitions.h#newcode270
src/flag-definitions.h:270: DEFINE_bool(optimize_constructed_arrays,
false,
On 2013/06/25 16:35:07, Hannes Payer wrote:
Should be true.
Cool, this flag is gone now.
https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc#newcode5885
src/hydrogen.cc:5885: Handle<FixedArray> literals(closure->literals(),
isolate());
On 2013/06/25 16:35:07, Hannes Payer wrote:
Literals can be removed.
Done.
https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc#newcode6018
src/hydrogen.cc:6018: // AllocationSite from the full code gen
On 2013/06/25 16:35:07, Hannes Payer wrote:
why can't you reuse the allocation site generated in full code gen?
The reason is that we don't create the AllocationSite at compile time,
we create it when we create the boilerplate object. So we don't have one
to look at if we don't have a boilerplate.
I guess we don't come into this case very much (where we are
crankshafting but apparently we never ran the code even once in full
codegen, which would have created the boilerplate).
https://codereview.chromium.org/15094018/diff/17001/src/objects-printer.cc
File src/objects-printer.cc (right):
https://codereview.chromium.org/15094018/diff/17001/src/objects-printer.cc#newcode1122
src/objects-printer.cc:1122: PrintF(out, " - payload: ");
On 2013/06/25 16:35:07, Hannes Payer wrote:
Why don't we call payload AllocationSite? That would carry more
information.
Done, but the type has to remain object because it can get cleared to
Undefined when type cells are cleared.
https://codereview.chromium.org/15094018/diff/17001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7462
src/objects.h:7462: DECL_ACCESSORS(payload, Object)
On 2013/06/25 16:35:07, Hannes Payer wrote:
Can we rename payload to something more meaningful? Like
transition_data...?
For this one I added a TODO because I'd have to touch a lot. I'd like to
do it in the follow up CL which renames AllocationSiteInfo to be called
AllocationMemento. That will be very simple, even though it touches a
lot.
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7467
src/objects.h:7467:
set_payload(Smi::FromInt(GetInitialFastElementsKind()));
On 2013/06/25 16:35:07, Hannes Payer wrote:
You can call SetElementsKindPayload there.
Done.
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7468
src/objects.h:7468: }
On 2013/06/25 16:35:07, Hannes Payer wrote:
declaration order: Typedefs and Enums, Constants (static const data
members),
Constructors, Destructor, Methods including static methods,
Data Members (except static const data members)
Moreover can we keep the static methods together?
I think I've arranged it better now, have a look.
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7491
src/objects.h:7491: return !payload()->IsSmi(); // payload is a
boilerplate
On 2013/06/25 16:35:07, Hannes Payer wrote:
remove comment or make it clear.
Clarified, and made the test stronger: return payload()->IsJSArray().
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7499
src/objects.h:7499: class AllocationSiteInfo: public Struct {
On 2013/06/25 16:35:07, Hannes Payer wrote:
declaration order... I just saw that it is not consistent in our code
base but
the google coding style guide suggest to do that. Let's be a good
example.
Done.
https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7511
src/objects.h:7511: static const int kPayloadOffset =
HeapObject::kHeaderSize;
On 2013/06/25 16:35:07, Hannes Payer wrote:
Do you think it makes sense to rename kPayloadOffset to
kAllocationSiteOffset? I
do not think that we are going to add more fields to
AllocationSiteInfo.
Done.
https://codereview.chromium.org/15094018/diff/17001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/15094018/diff/17001/src/runtime.cc#newcode532
src/runtime.cc:532: RUNTIME_FUNCTION(MaybeObject*,
Runtime_CreateArrayLiteralShallow) {
On 2013/06/25 16:35:07, Hannes Payer wrote:
Can we refactor Runtime_CreateArrayLiteral and
Runtime_CreateArrayLiteralShallow. They share quite some similarities.
thanks, good advice! Done
https://codereview.chromium.org/15094018/
--
--
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.
