Erik, thanks a lot for review, I'll respond in details somewhat later (in an hour or two). Just some minor observations. Regarding more generic approach: I thought of treading buffer to fill into string-specific write methods which would allow us to get rid of flattening completely. Probably would investigate it. And one thing I forgot in this version: I should shortcut copying after flattening---if string gets flat, it'd be possible to copy it with one move. Will update a CL when responding to your comments.
yours, anton. On Tue, Sep 29, 2009 at 12:40 PM, <[email protected]> wrote: > LGTM. I can't help thinking we could make this more general, but so far I > haven't come up with a nice scheme myself. > > > http://codereview.chromium.org/248031/diff/3001/3002 > File src/api.cc (right): > > http://codereview.chromium.org/248031/diff/3001/3002#newcode2448 > Line 2448: // TODO(antonm): consider some fancier ways (like Duff's). > Duff's Device is only useful for IO ports, not for real memory. > > http://codereview.chromium.org/248031/diff/3001/3002#newcode2480 > Line 2480: i::StringShape shape(*str); > I have been meaning to get rid of StringShape because the performance > benefits were very minimal while the risk of errors was rather large. > If the benefit here is measurable I would encourage you to make a new > scope for the shape so that it goes out of scope before the string > flattening below. This will make it harder for someone to accidentally > use it after a flattening or GC has made it invalid. > > http://codereview.chromium.org/248031/diff/3001/3002#newcode2483 > Line 2483: switch (shape.representation_tag()) { > You should switch on the full representation tag here so you avoid an if > in each case. > > http://codereview.chromium.org/248031/diff/3001/3002#newcode2499 > Line 2499: if (first_shape.representation_tag() == i::kSeqStringTag || > Consider making this a switch too. > > http://codereview.chromium.org/248031/diff/3001/3002#newcode2508 > Line 2508: if (second->length() == 0) { > How about 'int second_length = second->length()'. That saves you > looking it up more than once. Then you can do > if (second_length <= 1) { > bool only_from_first = (end < length - second_length) > and you can combine the two branches. > > http://codereview.chromium.org/248031 > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
