the idea is that often times in code, you will see something like this:...

the idea is that often times in code, you will see something like this:

lala = "<div>" +
	"<span>" +
		myvar +
	"</span>" +
"</div>";

this patch transforms that code during the parse phase to be:

lala = "<div><span>" + myvar + "</div></span>";

UPDATE: I merged the NewNumberLiteral in the switch case to be < 80 chars. please review my usage of NAN as 
default value. I also have had a horrible time getting these tools to work, and couldn't figure out how to 
add files (factory.cc/h) to the changeset. hopefully this works.

FIXED: brain fart on BinaryOperation... it's obvious I didn't run the tests :( - speaking of which, they 
don't compile for me cause of bad gcc 4.4 detection and hand fixing them is frustrating. that's gonna be my 
next patch... *sigh*

[Kevin Millikin (Chromium), 2009-10-01 12:11:15]

Here are some simple comments.  The general ones is that we cannot allocate
without handling allocation failures somehow, and we cannot skip the write
barrier on writes to the old generation unless it is guaranteed to be safe.

Please note that this doesn't do what you think it does.  Because addition
associates to the left, your example will be parsed as:

((("<div>" + "<span>") + myvar) + "</span>") + "</div>"

and folded to

"<div><span>" + myvar + "</span>" + "</div>"

http://codereview.chromium.org/243051/diff/2001/3001
File src/heap.cc (right):

http://codereview.chromium.org/243051/diff/2001/3001#newcode1638
Line 1638: PretenureFlag pretenure) {
Same parameter alignment rules for definitions as declarations.

http://codereview.chromium.org/243051/diff/2001/3001#newcode1695
Line 1695: Object* result = Allocate(map, pretenure ? LO_SPACE : NEW_SPACE);
1. You should not rely on NOT_PRETENURED being false and PRETENURED being true. 
Elsewhere in the file you will see:

AllocationSpace space = (pretenure == TENURED) ? ...
Object* result = Allocate(map, space);

2. A pretenured cons string should not go in LO_SPACE (the large object space
for objects bigger than ~8K).  It should go in OLD_POINTER_SPACE.

http://codereview.chromium.org/243051/diff/2001/3001#newcode1697
Line 1697: ASSERT(pretenure ? !InNewSpace(result) : InNewSpace(result));
Same comment about (not) relying on NOT_PRETENURED comparing as false and
PRETENURED comparing as true.

http://codereview.chromium.org/243051/diff/2001/3001#newcode1699
Line 1699: cons_string->set_first(first, SKIP_WRITE_BARRIER);
If the cons string was allocated in the old generation, it is not safe to skip
the write barrier here.

http://codereview.chromium.org/243051/diff/2001/3002
File src/heap.h (right):

http://codereview.chromium.org/243051/diff/2001/3002#newcode557
Line 557: static Object* AllocateConsString(String* first, String* second,
The normal style is one parameter per line if they don't fit on the first line. 
They should all be aligned:

static Object* AllocateConsString(String* first,
                                  String* second,
                                  PretenureFlag pretenure = NOT_TENURED);

or else

static Object* AllocateConsString(
    String* first,
    String* second,
    PretenureFlag pretenure = NOT_TENURED);

http://codereview.chromium.org/243051/diff/2001/3003
File src/parser.cc (right):

http://codereview.chromium.org/243051/diff/2001/3003#newcode2818
Line 2818: 
We do not allow lines that end in whitespace like this one.  This should be
flagged by tools/presubmit.py.

http://codereview.chromium.org/243051/diff/2001/3003#newcode2868
Line 2868: 
Another line that ends in whitespace (I don't think you need a blank line here
anyway).

http://codereview.chromium.org/243051/diff/2001/3003#newcode2869
Line 2869: Object* object = Heap::AllocateConsString(*str1, *str2, TENURED);
As indicated in the comment in heap.h, AllocateConsString can return a failure
(not safe to stick in a handle) and will not force a GC.  You should be calling
Factory::NewConsString.

[kenny, 2009-10-03 01:00:50]

On 2009/10/01 12:11:15, Kevin Millikin wrote:
> Please note that this doesn't do what you think it does.  Because addition
> associates to the left, your example will be parsed as:
> 
> ((("<div>" + "<span>") + myvar) + "</span>") + "</div>"
> 
> and folded to
> 
> "<div><span>" + myvar + "</span>" + "</div>"


you're absolutely right! holy shit man, good call... I've updated the diff to
account for BinaryOperations as well. I don't know if that part of the code is
very clear / optimized, if you could take a look at it. I couldn't find a way to
do it easily without duplicating code.

[kenny, 2009-10-03 01:01:34]

http://codereview.chromium.org/243051/diff/2001/3001
File src/heap.cc (right):

http://codereview.chromium.org/243051/diff/2001/3001#newcode1695
Line 1695: Object* result = Allocate(map, pretenure ? LO_SPACE : NEW_SPACE);
On 2009/10/01 12:11:15, Kevin Millikin wrote:
> 1. You should not rely on NOT_PRETENURED being false and PRETENURED being
true. 
> Elsewhere in the file you will see:
> 
> AllocationSpace space = (pretenure == TENURED) ? ...
> Object* result = Allocate(map, space);
> 
> 2. A pretenured cons string should not go in LO_SPACE (the large object space
> for objects bigger than ~8K).  It should go in OLD_POINTER_SPACE.

Done.

http://codereview.chromium.org/243051/diff/2001/3001#newcode1697
Line 1697: ASSERT(pretenure ? !InNewSpace(result) : InNewSpace(result));
On 2009/10/01 12:11:15, Kevin Millikin wrote:
> Same comment about (not) relying on NOT_PRETENURED comparing as false and
> PRETENURED comparing as true.

Done.

http://codereview.chromium.org/243051/diff/2001/3001#newcode1699
Line 1699: cons_string->set_first(first, SKIP_WRITE_BARRIER);
On 2009/10/01 12:11:15, Kevin Millikin wrote:
> If the cons string was allocated in the old generation, it is not safe to skip
> the write barrier here.

Done.

http://codereview.chromium.org/243051/diff/2001/3002
File src/heap.h (right):

http://codereview.chromium.org/243051/diff/2001/3002#newcode557
Line 557: static Object* AllocateConsString(String* first, String* second,
On 2009/10/01 12:11:15, Kevin Millikin wrote:
> The normal style is one parameter per line if they don't fit on the first
line. 
> They should all be aligned:
> 
> static Object* AllocateConsString(String* first,
>                                   String* second,
>                                   PretenureFlag pretenure = NOT_TENURED);
> 
> or else
> 
> static Object* AllocateConsString(
>     String* first,
>     String* second,
>     PretenureFlag pretenure = NOT_TENURED);

Done.

http://codereview.chromium.org/243051/diff/2001/3003
File src/parser.cc (right):

http://codereview.chromium.org/243051/diff/2001/3003#newcode2818
Line 2818: 
On 2009/10/01 12:11:15, Kevin Millikin wrote:
> We do not allow lines that end in whitespace like this one.  This should be
> flagged by tools/presubmit.py.

right now presubmit.py gives a crash for me. I'll try and get it working. Done.

http://codereview.chromium.org/243051/diff/2001/3003#newcode2868
Line 2868: 
On 2009/10/01 12:11:15, Kevin Millikin wrote:
> Another line that ends in whitespace (I don't think you need a blank line here
> anyway).

Done.

http://codereview.chromium.org/243051/diff/2001/3003#newcode2869
Line 2869: Object* object = Heap::AllocateConsString(*str1, *str2, TENURED);
On 2009/10/01 12:11:15, Kevin Millikin wrote:
> As indicated in the comment in heap.h, AllocateConsString can return a failure
> (not safe to stick in a handle) and will not force a GC.  You should be
calling
> Factory::NewConsString.

Done.

[Kevin Millikin (Chromium), 2009-10-03 07:08:26]

There are still some obvious problems.  I may have time to take a closer look
after the weekend if you have something new.

1. Factory::NewConsString doesn't take a pretenure flag, so the code in the
patch probably won't compile.  Perhaps you forgot to add factory.h and
factory.cc to the change list?

2. Your optimization should only be applied to addition, not *any* binary
operation with string operands.  In JS, "2" / "4" is 0.5, not "24".

[kenny, 2009-10-03 09:12:26]

On 2009/10/03 07:08:26, Kevin Millikin wrote:
> There are still some obvious problems.  I may have time to take a closer look
> after the weekend if you have something new.
> 
> 1. Factory::NewConsString doesn't take a pretenure flag, so the code in the
> patch probably won't compile.  Perhaps you forgot to add factory.h and
> factory.cc to the change list?

I did, but apparently you gatta edit the file manually when running gcl... now I
know :)


> 2. Your optimization should only be applied to addition, not *any* binary
> operation with string operands.  In JS, "2" / "4" is 0.5, not "24".

yea, that's a huge error on my part. fixed.

------------

now, like I said in my changelog, it's time for me to get all the tests running
smooth. I have compile errors out the wazoo with those cause of strict aliasing
and gcc 4.4. I also can't figure out how to run presubmit.py (fails on the popen
command to open cpplint.py) and neither can I figure out how to run the linting
thing...

man... so much stuff... lol. the problem is, this takes so much time to do, when
I have like 10 more optimizations that can be written burning through my head...
but I gatta wait, I think, until I've got the hang of it with these...

---------------

last thing, I noticed when I was running --print_code and stuff today that there
were multiple versions of that string in the reloc section. Is that because the
garbage collector hasn't cleaned up the second instances, or because I didn't
deallocate them properly?

thanks man
kenny

[Kevin Millikin (Chromium), 2009-10-09 04:23:24]

I glanced at this again.  General comments:

1. It is still wrong.  You did not address my previous comment: this
optimization only correctly applies to addition.

2. You have refactored the switch statement in the loop and introduced a bug. 
Please do not do that.  (Do not try to refactor code unless you understand it. 
Do not add new unrelated changes to a changelist *after* it has begun being
reviewed, as the reviewer may miss them.)

http://codereview.chromium.org/243051/diff/9/8003
File src/parser.cc (right):

http://codereview.chromium.org/243051/diff/9/8003#newcode2810
Line 2810: double new_number = NAN;
I find this new code less clear than the old because it's less direct.  If you
want to jump to the top of the loop ('continue'), why bother setting a variable,
jumping to the bottom of the switch, and then checking the variable?

http://codereview.chromium.org/243051/diff/9/8003#newcode2859
Line 2859: if (new_number != NAN) {
You have introduced a bug.  Because of the way NaN comparisons work,
*everything* is not equal to NaN (even NaN != NaN).  If you don't hit one of the
cases in the switch you will constant-fold to NaN.

1==1 should be true, not NaN.

http://codereview.chromium.org/243051/diff/9/8003#newcode2863
Line 2863: } else if (x_handle->IsString() && y_handle->IsString()) {
No, you cannot do this optimization unless op is Token::ADD.

http://codereview.chromium.org/243051/diff/9/8003#newcode2870
Line 2870: } else if (x && x->AsBinaryOperation() && y_handle->IsString()) {
You cannot do this optimization unless op is Token::Add.