First round of comments. Overall it looks good.
For the record (as discussed offline), in a future CL I'd like to see this
approach replaced by one that doesn't store magic numbers in the
typefeedback
cells; but rather generate specific array constructors for different
elements
kinds. Those array constructors point at their own fast-elements-kind map;
which
in turn has the main Array function as constructor. The constructor stored
in
the cell should only be internal and never leak to javascript, but allow us
to
use the constructor directly rather than special case the constructor code.
https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc
File src/builtins.cc (right):
https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode212
src/builtins.cc:212: }
Remove { ... }; replace ToObject(& by To(&; set the type of initial_map
as Map*; so you can remove the Map::cast below.
May even be you don't need any of this code. The ArrayFunction should
have an initial map due to bootstrapping.
https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode229
src/builtins.cc:229: if (holey) {
If this is holey, you'll probably want to update the type feedback.
Otherwise you'll create multiple objects with different maps, depending
on the size. Eg:
function f(i) { return new Array(i); } will create holey and non-holey
objects, depending on the calls to f. f(0) becomes non-holey, and f(5)
is holey. That's bad for the ICs coming later on.
https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode233
src/builtins.cc:233: if (IsMoreGeneralElementsKindTransition(kind,
to_kind)) {
Shouldn't happen I guess. Otherwise we are doing a lot of work to track
array types; while we end up ignoring it anyway.
https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode234
src/builtins.cc:234: maybe_array =
isolate->heap()->AllocateEmptyJSArray(to_kind);
Again; update type feedback in the cell.
https://codereview.chromium.org/11818021/diff/34001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/11818021/diff/34001/src/code-stubs.h#newcode1269
src/code-stubs.h:1269: int MinorKey() { return 0; }
I guess you don't need to overwrite the MinorKey?
https://codereview.chromium.org/11818021/diff/34001/src/elements.cc
File src/elements.cc (right):
https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode1990
src/elements.cc:1990: {
Remove { ... }.
https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode1997
src/elements.cc:1997: if (!maybe_obj->To(&fixed_array)) return
maybe_obj;
It seems like this method should be merged with
AllocateJSArrayAndStorage; if possible. That can be done in a future CL
though.
https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode2014
src/elements.cc:2014: { MaybeObject* maybe_obj = array->Initialize(0);
You don't need the { .. }; and To(& rather than ToObject(&.
https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode2023
src/elements.cc:2023: }
nit: I'd move this case above the == 1 case.
https://codereview.chromium.org/11818021/diff/34001/src/heap.cc
File src/heap.cc (right):
https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode3940
src/heap.cc:3940: { MaybeObject* maybe_result =
Remove {...}
https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode3944
src/heap.cc:3944: if (!maybe_result->ToObject(&result)) return
maybe_result;
->To(&
https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4223
src/heap.cc:4223: {
Remove {. ->To(&.
https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4261
src/heap.cc:4261:
ASSERT((*allocation_site_info_payload)->IsJSGlobalPropertyCell());
The cast below will take care of this assert.
https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4264
src/heap.cc:4264: ASSERT(cell->value()->IsSmi());
The cast below will take care of this assert.
https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4270
src/heap.cc:4270: ASSERT(initial_map != NULL);
Yeah, it seems like you should ensure that the map is created if it
isn't there yet. Otherwise we'll end up with NULL-pointers as maps...
https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4271
src/heap.cc:4271: //
constructor->set_initial_map(Map::cast(initial_map));
omit Map::cast.
https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4273
src/heap.cc:4273: mode = DONT_TRACK_ALLOCATION_SITE;
Weird;... you do all this work to set up the initial state; but then you
abort and set the mode to DONT_TRACK?
https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4317
src/heap.cc:4317: PrintF("Sorry, can't track yet in tenured space\n");
Euhm... Shouldn't we overwrite allocation_site_info_mode or so?
https://codereview.chromium.org/11818021/diff/34001/src/heap.h
File src/heap.h (right):
https://codereview.chromium.org/11818021/diff/34001/src/heap.h#newcode606
src/heap.h:606: // didn't include it because I don't see why we need it
yet.
Outdated comment?
https://codereview.chromium.org/11818021/diff/34001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):
https://codereview.chromium.org/11818021/diff/34001/src/ia32/builtins-ia32.cc#newcode1508
src/ia32/builtins-ia32.cc:1508: if (FLAG_optimize_constructed_arrays) {
Given that the 2 blocks of code are entirely independent, I'd split up
this function into 2 functions; and move the if to wherever this
function is being called.
https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32.cc#newcode51
src/ia32/code-stubs-ia32.cc:51: descriptor->stack_parameter_count_ =
NULL;
Unnecessary move.
https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32.cc#newcode61
src/ia32/code-stubs-ia32.cc:61: static Register registers[] = { eax, ebx
};
Please write a comment above identifying the purpose of these registers.
https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32.cc#newcode4856
src/ia32/code-stubs-ia32.cc:4856: __ j(equal, &done, Label::kFar);
I believe Label::kFar is the default, you can omit it if so.
https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32.cc#newcode4866
src/ia32/code-stubs-ia32.cc:4866: __ j(above, &miss, Label::kFar);
If you move this below the cmp, you can probably merge it with the
equivalent checks below.
https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32.cc#newcode4874
src/ia32/code-stubs-ia32.cc:4874: __ j(not_equal, &megamorphic_pre,
Label::kFar);
Jump to megamorphic directly.
https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32.cc#newcode4897
src/ia32/code-stubs-ia32.cc:4897: __ mov(ecx, FieldOperand(ecx,
GlobalObject::kGlobalContextOffset));
Check if this can be folded together with the code above.
https://codereview.chromium.org/11818021/diff/34001/src/isolate.cc
File src/isolate.cc (left):
https://codereview.chromium.org/11818021/diff/34001/src/isolate.cc#oldcode2080
src/isolate.cc:2080: deoptimizer_data_ = new DeoptimizerData;
Why did you move this?
https://codereview.chromium.org/11818021/diff/34001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode9364
src/objects.cc:9364: }
Seems like this is overlapping with AllocateJSArrayAndStorage.
https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode10562
src/objects.cc:10562: MaybeObject* ret = NULL;
Seems like you don't need this variable. Below you can just return NULL;
or return payload->TransitionElementsKind(to_kind).
https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode10580
src/objects.cc:10580: if (length <= 8*1024) {
Define a constant.
https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode10599
src/objects.cc:10599: IsFastObjectElementsKind(to_kind);
This heuristic seems very buried away. Can we make it more top-level
using some function? Don't you already have that somewhere?
https://codereview.chromium.org/11818021/diff/34001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/11818021/diff/34001/src/objects.h#newcode6986
src/objects.h:6986:
Remove.
https://codereview.chromium.org/11818021/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
