Function allocators handlified.

Function allocators, Heap::ReinitializeJSGlobalProxy() and Heap::ReinitializeJSReceiver() handlified. Factory::New<>() added.

R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20762

[Igor Sheludko, 2014-04-15 08:33:31]

PTAL

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc
File src/factory.cc (left):

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#oldcode1666
src/factory.cc:1666: Handle<JSFunction>
Factory::NewFunctionHelper(Handle<String> name,
one usage, inlined

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#oldcode1686
src/factory.cc:1686: Handle<JSFunction>
Factory::NewFunctionWithoutPrototypeHelper(
one usage, inlined

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1743
src/factory.cc:1743: Handle<JSFunction> Factory::NewFunctionHelper(Handle<Map>
function_map,
Handlified Heap::AllocateFunction()

https://codereview.chromium.org/237093006/diff/40001/src/factory.h
File src/factory.h (left):

https://codereview.chromium.org/237093006/diff/40001/src/factory.h#oldcode398
src/factory.h:398: Handle<JSFunction> NewFunctionWithoutPrototype(
moved declaration down

https://codereview.chromium.org/237093006/diff/40001/src/factory.h#oldcode402
src/factory.h:402: Handle<JSFunction> NewFunction(Handle<Object> super, bool
is_global);
declaration only

https://codereview.chromium.org/237093006/diff/40001/src/factory.h#oldcode404
src/factory.h:404: Handle<JSFunction> BaseNewFunctionFromSharedFunctionInfo(
one usage - inlined

https://codereview.chromium.org/237093006/diff/40001/src/factory.h#oldcode469
src/factory.h:469: Handle<JSFunction> NewFunction(Handle<String> name,
Moved these declarations up

https://codereview.chromium.org/237093006/diff/40001/src/factory.h#oldcode475
src/factory.h:475: Handle<JSFunction> NewFunction(Handle<Map> function_map,
declaration only

[Michael Starzinger, 2014-04-15 09:54:59]

I like where this is going, some issues though.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1587
src/factory.cc:1587: Heap* heap = isolate()->heap();
nit: There should be Factory::heap() IIRC, I think there is no need to pull out
the Heap into a local variable if we use that one.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1589
src/factory.cc:1589: heap->InitializeJSObjectFromMap(*jsobj,
FixedArray::cast(*properties), *map);
nit: No need to cast the FixedArray anymore, it is also typed correctly.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1600
src/factory.cc:1600: InitializeFunction(js_function, shared, the_hole_value());
Same comment as below goes for the call to InitializeFunction. This whole method
needs to be re-ordered to maintain a consistent heap at allocation points.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1606
src/factory.cc:1606: heap->CreateFillerObjectAt(
Creating the filler object needs to happen right after we change the map of the
object, otherwise the heap is no longer iterable. Hence the allocations in the
block above could cause GCs that see an inconsistent heap. Please move this up
to after the call to InitializeJSObjectFromMap above.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1625
src/factory.cc:1625: isolate()->factory()->NewFixedArray(prop_size, TENURED);
nit: We are already inside the Factory, just call NewFixedArray directly here.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1630
src/factory.cc:1630: Heap* heap = isolate()->heap();
nit: There should be Factory::heap() IIRC, I think there is no need to pull out
the Heap into a local variable if we use that one.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1632
src/factory.cc:1632: heap->InitializeJSObjectFromMap(*object,
FixedArray::cast(*properties), *map);
nit: No need to cast the FixedArray anymore, it is also typed correctly.

https://codereview.chromium.org/237093006/diff/40001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/237093006/diff/40001/src/factory.h#newcode403
src/factory.h:403: Handle<JSFunction> constructor, Handle<JSGlobalProxy>
global);
suggestion: I know it was like this before your change, but I think it might
make more sense to have "global" be the first argument. WDYT?

https://codereview.chromium.org/237093006/diff/40001/src/factory.h#newcode602
src/factory.h:602: // Creates a heap object based on the map.
nit: Can we add one sentence saying that the fields of the heap object are not
initialized by this function? It's the responsibility of the caller to do that.

[Igor Sheludko, 2014-04-15 11:03:41]

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1587
src/factory.cc:1587: Heap* heap = isolate()->heap();
On 2014/04/15 09:54:59, Michael Starzinger wrote:
> nit: There should be Factory::heap() IIRC, I think there is no need to pull
out
> the Heap into a local variable if we use that one.

There's no heap() method. Lets address this in a separate CL.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1589
src/factory.cc:1589: heap->InitializeJSObjectFromMap(*jsobj,
FixedArray::cast(*properties), *map);
On 2014/04/15 09:54:59, Michael Starzinger wrote:
> nit: No need to cast the FixedArray anymore, it is also typed correctly.

Done.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1600
src/factory.cc:1600: InitializeFunction(js_function, shared, the_hole_value());
On 2014/04/15 09:54:59, Michael Starzinger wrote:
> Same comment as below goes for the call to InitializeFunction. This whole
method
> needs to be re-ordered to maintain a consistent heap at allocation points.

Done.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1606
src/factory.cc:1606: heap->CreateFillerObjectAt(
On 2014/04/15 09:54:59, Michael Starzinger wrote:
> Creating the filler object needs to happen right after we change the map of
the
> object, otherwise the heap is no longer iterable. Hence the allocations in the
> block above could cause GCs that see an inconsistent heap. Please move this up
> to after the call to InitializeJSObjectFromMap above.

I fixed this in a little bit different way than you suggested.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1625
src/factory.cc:1625: isolate()->factory()->NewFixedArray(prop_size, TENURED);
On 2014/04/15 09:54:59, Michael Starzinger wrote:
> nit: We are already inside the Factory, just call NewFixedArray directly here.

Fixed here and in all other places in the file.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1630
src/factory.cc:1630: Heap* heap = isolate()->heap();
On 2014/04/15 09:54:59, Michael Starzinger wrote:
> nit: There should be Factory::heap() IIRC, I think there is no need to pull
out
> the Heap into a local variable if we use that one.

Done.

https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1632
src/factory.cc:1632: heap->InitializeJSObjectFromMap(*object,
FixedArray::cast(*properties), *map);
On 2014/04/15 09:54:59, Michael Starzinger wrote:
> nit: No need to cast the FixedArray anymore, it is also typed correctly.

Done.

https://codereview.chromium.org/237093006/diff/40001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/237093006/diff/40001/src/factory.h#newcode403
src/factory.h:403: Handle<JSFunction> constructor, Handle<JSGlobalProxy>
global);
On 2014/04/15 09:54:59, Michael Starzinger wrote:
> suggestion: I know it was like this before your change, but I think it might
> make more sense to have "global" be the first argument. WDYT?

I agree, done.

https://codereview.chromium.org/237093006/diff/40001/src/factory.h#newcode602
src/factory.h:602: // Creates a heap object based on the map.
On 2014/04/15 09:54:59, Michael Starzinger wrote:
> nit: Can we add one sentence saying that the fields of the heap object are not
> initialized by this function? It's the responsibility of the caller to do
that.

Done.

[Michael Starzinger, 2014-04-15 11:10:47]

LGTM. I like this.

[Igor Sheludko, 2014-04-15 11:51:43]

Committed patchset #4 manually as r20762 (presubmit successful).