Some optimizations for packer.js.

Some optimizations for dictionary use of objects.
Committed: http://code.google.com/p/v8/source/detail?r=3336

[Søren Thygesen Gjesse, 2009-11-19 21:42:56]

I don't think we should mention what specific JavaScript code an optimization is
targeted at (packer.js) in the source comments, but state why it is a good idea
in more general terms.

I have just done StringAdd in native code (with the changed string
representation separating length and hash), so we need to figure out whether to
handle two char strings in the runtime system still or do hash calculation and
symbol table lookup (perhaps just a single probe) in native code if to does not
get to complicated and error phrone.

http://codereview.chromium.org/409007/diff/1/2
File src/heap.cc (right):

http://codereview.chromium.org/409007/diff/1/2#newcode1784
Line 1784: String* symbol;
Please add a comment on why digits only will not work here.

http://codereview.chromium.org/409007/diff/1/2#newcode1785
Line 1785: if ((c1 - '0' > '9' - '0' ||
Why c1 - '0' > '9' - '0' instead of c1 > '9'?

http://codereview.chromium.org/409007/diff/1/2#newcode1878
Line 1878: } else if (length == 2) {
Refactor duplicated code.

http://codereview.chromium.org/409007/diff/1/7
File src/heap.h (right):

http://codereview.chromium.org/409007/diff/1/7#newcode634
Line 634: static bool LookupTwoCharsIfExists(String* str, String** symbol);
LookupTwoCharsIfExists -> LookupTwoCharsSymbolIfExists to match the other symbol
table lookup functions. Comment that it does not work for digits.

http://codereview.chromium.org/409007/diff/1/6
File src/objects-inl.h (right):

http://codereview.chromium.org/409007/diff/1/6#newcode3113
Line 3113: const int kArraySizeThatFitsComfortablyInNewSpace = 128;
This does not take the calculation required_size + (required_size >> 3) in
Expand into account...

http://codereview.chromium.org/409007/diff/1/6#newcode3114
Line 3114: if (required_size < kArraySizeThatFitsComfortablyInNewSpace) {
Maybe combine these two if's into one if condition. Also make a note that
"Expand" will always allocate a new backing store even if it is not expanding.

http://codereview.chromium.org/409007/diff/1/3
File src/objects.cc (right):

http://codereview.chromium.org/409007/diff/1/3#newcode420
Line 420: // constantly reallocating them.
This code is duplicated below, please refactor.

http://codereview.chromium.org/409007/diff/1/3#newcode7350
Line 7350: : c1_(c1), c2_(c2) {
How about ASSERT(c1 > '9' || c2 > '9') here? I see that the ASSERT_EQ below will
catch it, but it might be a bit obscure why.

http://codereview.chromium.org/409007/diff/1/4
File src/runtime.cc (right):

http://codereview.chromium.org/409007/diff/1/4#newcode2361
Line 2361: if (from->IsSmi() && to->IsSmi()) {
Maybe add a comment to this fast case.

http://codereview.chromium.org/409007/diff/1/4#newcode2364
Line 2364: 
Please move the declaration of start and end before the if and the following
four lines after the else block to avoid commin code in if and else block.

[Erik Corry, 2009-11-20 10:12:46]

http://codereview.chromium.org/409007/diff/1/2
File src/heap.cc (right):

http://codereview.chromium.org/409007/diff/1/2#newcode1785
Line 1785: if ((c1 - '0' > '9' - '0' ||
On 2009/11/19 21:42:57, Søren Gjesse wrote:
> Why c1 - '0' > '9' - '0' instead of c1 > '9'?

I moved this to a different function so I could point out in a comment that the
variables are unsigned and thus what you suggest would be completely different.

http://codereview.chromium.org/409007/diff/1/2#newcode1878
Line 1878: } else if (length == 2) {
On 2009/11/19 21:42:57, Søren Gjesse wrote:
> Refactor duplicated code.

Done.

http://codereview.chromium.org/409007/diff/1/6
File src/objects-inl.h (right):

http://codereview.chromium.org/409007/diff/1/6#newcode3113
Line 3113: const int kArraySizeThatFitsComfortablyInNewSpace = 128;
On 2009/11/19 21:42:57, Søren Gjesse wrote:
> This does not take the calculation required_size + (required_size >> 3) in
> Expand into account...

Since it's a number I pulled out of my backside I don't think that matters.

http://codereview.chromium.org/409007/diff/1/6#newcode3114
Line 3114: if (required_size < kArraySizeThatFitsComfortablyInNewSpace) {
On 2009/11/19 21:42:57, Søren Gjesse wrote:
> Maybe combine these two if's into one if condition. Also make a note that
> "Expand" will always allocate a new backing store even if it is not expanding.

Done.

http://codereview.chromium.org/409007/diff/1/3
File src/objects.cc (right):

http://codereview.chromium.org/409007/diff/1/3#newcode420
Line 420: // constantly reallocating them.
On 2009/11/19 21:42:57, Søren Gjesse wrote:
> This code is duplicated below, please refactor.

Instead I removed this occurrence since it is very rarely hit.

http://codereview.chromium.org/409007/diff/1/3#newcode7350
Line 7350: : c1_(c1), c2_(c2) {
On 2009/11/19 21:42:57, Søren Gjesse wrote:
> How about ASSERT(c1 > '9' || c2 > '9') here? I see that the ASSERT_EQ below
will
> catch it, but it might be a bit obscure why.

Added a comment to the ASSERT_EQ below instead.