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.
https://codereview.chromium.org/237093006/
--
--
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/d/optout.
