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

runtime/vm/symbols.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/symbols.h"
 
 #include "vm/handles.h"
 #include "vm/handles_impl.h"
 #include "vm/hash_table.h"
 #include "vm/isolate.h"
@@ skipping 375 matching lines @@
   if (str1.Length() == 0) {
     return New(str2);
   } else if (str2.Length() == 0) {
     return New(str1);
   } else {
     return NewSymbol(ConcatString(str1, str2));
   }
 }
 
 
+RawString* Symbols::FromConcatAll(const GrowableArray<const String*>& strs) {
+  intptr_t len_sum = 0;
+  for (intptr_t i = 0; i < strs.length(); i++) {
+    len_sum += OS::SNPrint(NULL, 0, "%s", strs[i]->ToCString());

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

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.)

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

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

[koda, 2015/08/26 20:02:03]

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.

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

I'll leave it as it is.

+  }
+  Zone* zone = Thread::Current()->zone();
+  intptr_t buffer_len = len_sum + 1;  // Space for terminating character.
+  char* buffer = zone->Alloc<char>(buffer_len);
+  char* orig_buffer = buffer;

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

const char* const orig_buffer

[koda, 2015/08/26 20:02:03]

Ditto?

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

Done.

+  for (intptr_t i = 0; i < strs.length(); i++) {
+    intptr_t len = OS::SNPrint(buffer, buffer_len, "%s", strs[i]->ToCString());
+    buffer += len;
+    buffer_len -= len;
+    ASSERT(buffer_len >= 0);
+  }
+  return Symbols::New(orig_buffer, len_sum);

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

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.)

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

If you decide to overestimate the buffer size, you can replace 'len_sum' with
'buffer - orig_buffer' here.

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

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. 

+}
+
+
 // StringType can be StringSlice, ConcatString, or {Latin1,UTF16,UTF32}Array.
 template<typename StringType>
 RawString* Symbols::NewSymbol(const StringType& str) {
   Thread* thread = Thread::Current();
   Isolate* isolate = thread->isolate();
   Zone* zone = thread->zone();
   String& symbol = String::Handle(zone);
   {
     Isolate* vm_isolate = Dart::vm_isolate();
     SymbolTable table(zone, vm_isolate->object_store()->symbol_table());
@@ skipping 63 matching lines @@
 RawObject* Symbols::GetVMSymbol(intptr_t object_id) {
   ASSERT(IsVMSymbolId(object_id));
   intptr_t i = (object_id - kMaxPredefinedObjectIds);
   if ((i > kIllegal) && (i < Symbols::kMaxPredefinedId)) {
     return symbol_handles_[i]->raw();
   }
   return Object::null();
 }
 
 }  // namespace dart