Issue 10915260: Reduce space usage of simple transitions and descriptors holders.

Issue 10915260: Reduce space usage of simple transitions and descriptors holders. (Closed)

Created:
8 years, 3 months ago by Toon Verwaest

Modified:
8 years, 3 months ago

Reviewers:
Michael Starzinger

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Reduce space usage of simple transitions and descriptors holders. Committed: https://code.google.com/p/v8/source/detail?r=12544

Patch Set 3 : Addressed comments #

Created: 8 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+165 lines, -47 lines)			Patch
M	src/objects.h	View	1 2	3 chunks	+15 lines, -2 lines	0 comments	Download
M	src/objects.cc	View	1 2	8 chunks	+16 lines, -9 lines	0 comments	Download
M	src/objects-inl.h	View	1	5 chunks	+32 lines, -13 lines	0 comments	Download
M	src/objects-visiting-inl.h	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
M	src/transitions.h	View		4 chunks	+24 lines, -4 lines	0 comments	Download
M	src/transitions.cc	View		3 chunks	+52 lines, -15 lines	0 comments	Download
M	src/transitions-inl.h	View	1 2	5 chunks	+23 lines, -4 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Michael Starzinger

LGTM (a few comments, but mainly style issues). https://chromiumcodereview.appspot.com/10915260/diff/16001/src/objects.cc File src/objects.cc (right): https://chromiumcodereview.appspot.com/10915260/diff/16001/src/objects.cc#newcode7563 src/objects.cc:7563: RightTrimFixedArray<FROM_GC>(heap, ...

8 years, 3 months ago (2012-09-19 09:34:55 UTC) #2

Toon Verwaest

8 years, 3 months ago (2012-09-19 09:53:23 UTC) #3

Addressed comments.

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/objects.cc#new...
src/objects.cc:7563: RightTrimFixedArray<FROM_GC>(heap, t, trim);
On 2012/09/19 09:34:55, Michael Starzinger wrote:
> Can we use "t->IsSimpleTransition() ? trim : trim *
> TransitionArray::kTransitionSize" here?

Done.

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/objects.h#newc...
src/objects.h:179: 
On 2012/09/19 09:34:55, Michael Starzinger wrote:
> Insert second empty newline here.

Done.

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/objects.h#newc...
src/objects.h:180: // Indicates whether the transition is simple: it either adds
a property to a
On 2012/09/19 09:34:55, Michael Starzinger wrote:
> The comment is slightly confusing to me. Can we rephrase the second part? What
> is "it" in this sentence?

Done.

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/objects.h#newc...
src/objects.h:186: 
On 2012/09/19 09:34:55, Michael Starzinger wrote:
> Insert second empty newline here.

Done.

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/objects.h#newc...
src/objects.h:4849: SimpleTransitionFlag flag = SIMPLE_TRANSITION);
On 2012/09/19 09:34:55, Michael Starzinger wrote:
> The Google C++ Style Guide doesn't like default arguments. We sometimes break
> that rule. But since this method only has two call-sites, I think it's not
worth
> here.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

Done.

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/objects.h#newc...
src/objects.h:5001: int descriptor_index = 0);
On 2012/09/19 09:34:55, Michael Starzinger wrote:
> Likewise. Although there are more call-sites here. But they are all in
> objects.cc, so I also think it's not worth here.

Done.

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/transitions-inl.h
File src/transitions-inl.h (right):

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/transitions-in...
src/transitions-inl.h:73: return get(kElementsTransitionIndex) !=
Smi::FromInt(0);
On 2012/09/19 09:34:55, Michael Starzinger wrote:
> Can we turn that into ...
> 
> return IsFullTransitionArray() &&
>     get(kElementsTransitionIndex) != Smi::FromInt(0);

Done.

https://chromiumcodereview.appspot.com/10915260/diff/16001/src/transitions-in...
src/transitions-inl.h:128: return prototype_transitions != Smi::FromInt(0);
On 2012/09/19 09:34:55, Michael Starzinger wrote:
> Can we turn that into ...
> 
> return IsFullTransitionArray() &&
>     get(kPrototypeTransitionsIndex) != Smi::FromInt(0);

Done.

Expand Messages | Collapse Messages