Guys, I've got another CL solving the same problem: http://codereview.chromium.org/255004
You're all should be on reviewers list. The results are mostly the same: http://dromaeo.com/?id=74679,74680,74694 (baseline, this CL, new one). I'm going to address flattening (that might be helpful for some cases) with explicit API call if string is not externalized. yours, anton. On Tue, Sep 29, 2009 at 2:09 PM, Erik Corry <[email protected]> wrote: > > > 2009/9/29 Anton Muhin <[email protected]> >> >> 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. > > You could use FlattenString from handles.h. Then you can be sure that it is > flat afterwards. > >> >> 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 >> > > > > > -- > Erik Corry, Software Engineer > Google Denmark ApS. CVR nr. 28 86 69 84 > c/o Philip & Partners, 7 Vognmagergade, P.O. Box 2227, DK-1018 Copenhagen K, > Denmark. > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
