Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(269)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 10692130: Swap bitfield3 and backpointer. (Closed)

Created:
8 years, 5 months ago by Toon Verwaest
Modified:
8 years, 5 months ago
Reviewers:
Jakob Kummerow
CC:
v8-dev
Visibility:
Public.

Description

Swap bitfield3 and backpointer. Bitfield3 now has its own field, while the backpointer shares the field with the descriptor array; which will become the transition array. BUG= TEST= Committed: https://code.google.com/p/v8/source/detail?r=12034

Patch Set 1 : u #

Total comments: 24
Unified diffs Side-by-side diffs Delta from patch set Stats (+219 lines, -140 lines) Patch
M src/arm/full-codegen-arm.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M src/arm/lithium-arm.h View 1 chunk +4 lines, -2 lines 0 comments Download
M src/arm/lithium-arm.cc View 1 chunk +2 lines, -1 line 0 comments Download
M src/arm/lithium-codegen-arm.cc View 1 chunk +2 lines, -1 line 0 comments Download
M src/arm/macro-assembler-arm.h View 2 chunks +6 lines, -1 line 0 comments Download
M src/arm/macro-assembler-arm.cc View 3 chunks +42 lines, -7 lines 0 comments Download
M src/heap.cc View 4 chunks +2 lines, -5 lines 0 comments Download
M src/ia32/macro-assembler-ia32.h View 1 chunk +3 lines, -0 lines 0 comments Download
M src/ia32/macro-assembler-ia32.cc View 3 chunks +37 lines, -7 lines 6 comments Download
M src/mark-compact.cc View 1 chunk +6 lines, -4 lines 2 comments Download
M src/objects.h View 7 chunks +12 lines, -24 lines 4 comments Download
M src/objects.cc View 4 chunks +2 lines, -10 lines 0 comments Download
M src/objects-inl.h View 6 chunks +54 lines, -65 lines 12 comments Download
M src/profile-generator.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/runtime.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M src/x64/macro-assembler-x64.h View 1 chunk +3 lines, -0 lines 0 comments Download
M src/x64/macro-assembler-x64.cc View 3 chunks +37 lines, -6 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Toon Verwaest
PTAL. The code generation is slightly more complex since the descriptor array is not shared ...
8 years, 5 months ago (2012-07-09 13:53:57 UTC) #1
Jakob Kummerow
LGTM with comments. The comments for ia32 apply to the other architectures as well -- ...
8 years, 5 months ago (2012-07-10 12:26:25 UTC) #2
Toon Verwaest
8 years, 5 months ago (2012-07-10 13:28:28 UTC) #3 
Addressed comments.

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-asse...
File src/ia32/macro-assembler-ia32.cc (right):

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-asse...
src/ia32/macro-assembler-ia32.cc:2548: AbortIfNotFixedArray(descriptors);
On 2012/07/10 12:26:25, Jakob wrote:
> This check is pretty pointless, don't you think? You only get here if you
either
> just passed the fixed_array_map() check, or after loading
> empty_descriptor_array() (which we can probably trust to be a FixedArray; if
you
> think we can't, an
> ASSERT(isolate->factory()->empty_descriptor_array()->IsFixedArray() at
> crankshafting time would be quite sufficient).

Done.

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-asse...
src/ia32/macro-assembler-ia32.cc:2548: AbortIfNotFixedArray(descriptors);
On 2012/07/10 12:26:25, Jakob wrote:
> This check is pretty pointless, don't you think? You only get here if you
either
> just passed the fixed_array_map() check, or after loading
> empty_descriptor_array() (which we can probably trust to be a FixedArray; if
you
> think we can't, an
> ASSERT(isolate->factory()->empty_descriptor_array()->IsFixedArray() at
> crankshafting time would be quite sufficient).

Done.

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-asse...
src/ia32/macro-assembler-ia32.cc:2919: AbortIfNotFixedArray(edx);
On 2012/07/10 12:26:25, Jakob wrote:
> This check is entirely pointless. We just did a map check on edx.

Done.

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-asse...
src/ia32/macro-assembler-ia32.cc:2919: AbortIfNotFixedArray(edx);
On 2012/07/10 12:26:25, Jakob wrote:
> This check is entirely pointless. We just did a map check on edx.

Done.

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/mark-compact.cc...
src/mark-compact.cc:1837: ASSERT(descriptor_array->IsMap() ||
descriptor_array->IsUndefined());
It's already marked above, by accessing map->GetBackPointer().

On 2012/07/10 12:26:25, Jakob wrote:
> When descriptor_array->IsMap(), then it's the back pointer, right? How about
> marking that (as the comment above says)?

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#n...
src/objects-inl.h:3420: ASSERT(!value->IsFailure());
Removed.

On 2012/07/10 12:26:25, Jakob wrote:
> Can this happen?

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#n...
src/objects-inl.h:3421: Object* object = READ_FIELD(this,
kInstanceDescriptorsOrBackPointerOffset);
On 2012/07/10 12:26:25, Jakob wrote:
> Looks like you can move this into the "else" block below.

Done.

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#n...
src/objects-inl.h:3442: int Map::bit_field3() {
On 2012/07/10 12:26:25, Jakob wrote:
> SMI_ACCESSORS(Map, bit_field3, kBitField3Offset)
> should take care of both this and the setter below.

Done.

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#n...
src/objects-inl.h:3501: // descriptor array that will contain an element
transition.
On 2012/07/10 12:26:25, Jakob wrote:
> nit: s/element/elements/

Done.

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#n...
src/objects-inl.h:3609: ASSERT(!value->IsFailure());
Removed.

On 2012/07/10 12:26:25, Jakob wrote:
> Can this happen?

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#n...
src/objects-inl.h:3613: } else {
Then we just overwrite it, since it's the default back-pointer value.

On 2012/07/10 12:26:25, Jakob wrote:
> What if (object->IsUndefined()) ?

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects.h#newco...
src/objects.h:4617: // TODO(1399): It should be possible to make room for
bit_field3 in the map
On 2012/07/10 12:26:25, Jakob wrote:
> outdated comment?

Done.

https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects.h#newco...
src/objects.h:4980: // TODO(1399): It should be possible to make room for
bit_field3 in the map
On 2012/07/10 12:26:25, Jakob wrote:
> outdated comment?

Done.

Powered by Google App Engine
This is Rietveld 408576698