Use zone allocated C-strings instead of String heap objects to construct signature symbols

Use zone allocated C-strings instead of String heap objects to construct signature symbols

BUG=
R=koda@google.com

Committed: https://github.com/dart-lang/sdk/commit/b5c256b85f4b0f01521c74a512b6808484da7ea3

[srdjan, 2015-08-26 16:45:35]

srdjan@google.com changed reviewers:
+ koda@google.com

[koda, 2015-08-26 17:37:06]

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:6480: GrowableArray<const String*> pieces(4);
Nit, but wouldn't 3 or 5 make more sense? E.g.,
{foo} = 3 pieces, or
{foo, bar} = 5 pieces
In any case, please add a comment about what is the expected most common form of
signature.

(I suppose you could even compute the needed length beforehand, but that might
complicate the code.)

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:6593: GrowableArray<const String*> pieces(zone, 4);
Ditto about choosing most common case as default.

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc
File runtime/vm/symbols.cc (right):

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc#...
runtime/vm/symbols.cc:399: len_sum += OS::SNPrint(NULL, 0, "%s",
strs[i]->ToCString());
There's a lot of temporary allocations and duplicate work here. Some suggestions
for your consideration:

1. Overestimate the buffer length, since it's a temporary zone allocation
anyway.

2. Cache the results from ToCString to reduce duplicate work and temporary
allocations.

3. Use Utf8::Length or plain strlen instead of SNPrint.

(It would be nice if we could use String::ToUTF8 instead, but then we should
probably first move the "one-byte char fast case" from String::ToCString into
String::ToUTF8.)

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc#...
runtime/vm/symbols.cc:404: char* orig_buffer = buffer;
const char* const orig_buffer

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc#...
runtime/vm/symbols.cc:411: return Symbols::New(orig_buffer, len_sum);
What kind of hit ratio do we expect here? (If it's high, we could consider
adding ability to perform lookup from the pieces without materializing the
concatenation.)

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc#...
runtime/vm/symbols.cc:411: return Symbols::New(orig_buffer, len_sum);
If you decide to overestimate the buffer size, you can replace 'len_sum' with
'buffer - orig_buffer' here.

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.h#n...
runtime/vm/symbols.h:9: #include "vm/snapshot_ids.h"
Include growable_array.h

[srdjan, 2015-08-26 19:43:55]

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:6480: GrowableArray<const String*> pieces(4);
On 2015/08/26 17:37:06, koda wrote:
> Nit, but wouldn't 3 or 5 make more sense? E.g.,
> {foo} = 3 pieces, or
> {foo, bar} = 5 pieces
> In any case, please add a comment about what is the expected most common form
of
> signature.
> 
> (I suppose you could even compute the needed length beforehand, but that might
> complicate the code.)

Set to 5 and added comment.

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:6593: GrowableArray<const String*> pieces(zone, 4);
On 2015/08/26 17:37:06, koda wrote:
> Ditto about choosing most common case as default.

Here it is not that clear/typical ('(dynamic) => dynamic' -> 4 pieces). Not
changing anything.

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc
File runtime/vm/symbols.cc (right):

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc#...
runtime/vm/symbols.cc:399: len_sum += OS::SNPrint(NULL, 0, "%s",
strs[i]->ToCString());
On 2015/08/26 17:37:06, koda wrote:
> There's a lot of temporary allocations and duplicate work here. Some
suggestions
> for your consideration:
> 
> 1. Overestimate the buffer length, since it's a temporary zone allocation
> anyway.
> 
> 2. Cache the results from ToCString to reduce duplicate work and temporary
> allocations.
> 
> 3. Use Utf8::Length or plain strlen instead of SNPrint.
> 
> (It would be nice if we could use String::ToUTF8 instead, but then we should
> probably first move the "one-byte char fast case" from String::ToCString into
> String::ToUTF8.)

1. Overestimate the buffer length: there is always the danger (synthetic code),
that we do not allocate enough space.

2.  & 3. are both good suggestions: holding onto the cons char* generated by
ToCString and using strlen

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc#...
runtime/vm/symbols.cc:411: return Symbols::New(orig_buffer, len_sum);
On 2015/08/26 17:37:06, koda wrote:
> What kind of hit ratio do we expect here? (If it's high, we could consider
> adding ability to perform lookup from the pieces without materializing the
> concatenation.)

Added a TODO in case this turns out to be hot code. Original code generated
Strings instead of Symbols. From reading the code, it makes sense to me to make
the signatures symbols.

[koda, 2015-08-26 20:02:04]

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc
File runtime/vm/symbols.cc (right):

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc#...
runtime/vm/symbols.cc:399: len_sum += OS::SNPrint(NULL, 0, "%s",
strs[i]->ToCString());
On 2015/08/26 19:43:55, srdjan wrote:
> On 2015/08/26 17:37:06, koda wrote:
> > There's a lot of temporary allocations and duplicate work here. Some
> suggestions
> > for your consideration:
> > 
> > 1. Overestimate the buffer length, since it's a temporary zone allocation
> > anyway.
> > 
> > 2. Cache the results from ToCString to reduce duplicate work and temporary
> > allocations.
> > 
> > 3. Use Utf8::Length or plain strlen instead of SNPrint.
> > 
> > (It would be nice if we could use String::ToUTF8 instead, but then we should
> > probably first move the "one-byte char fast case" from String::ToCString
into
> > String::ToUTF8.)
> 
> 1. Overestimate the buffer length: there is always the danger (synthetic
code),
> that we do not allocate enough space.

I don't mean "a large enough constant".

I mean compute 2 x (sum of one-byte string lengths) + 4 x (sum of two-byte
string lengths).

I'll leave it up to you.

> 
> 2.  & 3. are both good suggestions: holding onto the cons char* generated by
> ToCString and using strlen

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc#...
runtime/vm/symbols.cc:404: char* orig_buffer = buffer;
On 2015/08/26 17:37:06, koda wrote:
> const char* const orig_buffer

Ditto?

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.h#n...
runtime/vm/symbols.h:9: #include "vm/snapshot_ids.h"
On 2015/08/26 17:37:06, koda wrote:
> Include growable_array.h

Any reason not to do this?

[srdjan, 2015-08-26 20:23:42]

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc
File runtime/vm/symbols.cc (right):

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc#...
runtime/vm/symbols.cc:399: len_sum += OS::SNPrint(NULL, 0, "%s",
strs[i]->ToCString());
On 2015/08/26 20:02:03, koda wrote:
> On 2015/08/26 19:43:55, srdjan wrote:
> > On 2015/08/26 17:37:06, koda wrote:
> > > There's a lot of temporary allocations and duplicate work here. Some
> > suggestions
> > > for your consideration:
> > > 
> > > 1. Overestimate the buffer length, since it's a temporary zone allocation
> > > anyway.
> > > 
> > > 2. Cache the results from ToCString to reduce duplicate work and temporary
> > > allocations.
> > > 
> > > 3. Use Utf8::Length or plain strlen instead of SNPrint.
> > > 
> > > (It would be nice if we could use String::ToUTF8 instead, but then we
should
> > > probably first move the "one-byte char fast case" from String::ToCString
> into
> > > String::ToUTF8.)
> > 
> > 1. Overestimate the buffer length: there is always the danger (synthetic
> code),
> > that we do not allocate enough space.
> 
> I don't mean "a large enough constant".
> 
> I mean compute 2 x (sum of one-byte string lengths) + 4 x (sum of two-byte
> string lengths).
> 
> I'll leave it up to you.
> 
> > 
> > 2.  & 3. are both good suggestions: holding onto the cons char* generated by
> > ToCString and using strlen
> 

I'll leave it as it is.

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.cc#...
runtime/vm/symbols.cc:404: char* orig_buffer = buffer;
On 2015/08/26 20:02:03, koda wrote:
> On 2015/08/26 17:37:06, koda wrote:
> > const char* const orig_buffer
> 
> Ditto?

Done.

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/1313363005/diff/100001/runtime/vm/symbols.h#n...
runtime/vm/symbols.h:9: #include "vm/snapshot_ids.h"
On 2015/08/26 20:02:03, koda wrote:
> On 2015/08/26 17:37:06, koda wrote:
> > Include growable_array.h
> 
> Any reason not to do this?

Added.

[koda, 2015-08-26 20:26:18]

lgtm

[srdjan, 2015-08-26 20:31:18]

Committed patchset #8 (id:140001) manually as
b5c256b85f4b0f01521c74a512b6808484da7ea3 (presubmit successful).