Revert OneByteString back to ISO Latin-1 instead of ASCII

runtime/vm/dart_api_message.cc

Index: runtime/vm/dart_api_message.cc
===================================================================
--- runtime/vm/dart_api_message.cc	(revision 14917)
+++ runtime/vm/dart_api_message.cc	(working copy)
@@ -347,13 +347,22 @@
       intptr_t len = ReadSmiValue();
       intptr_t hash = ReadSmiValue();
       USE(hash);
-      Dart_CObject* object = AllocateDartCObjectString(len);
+      uint8_t *latin1 =
+          reinterpret_cast<uint8_t*>(::malloc(len * sizeof(uint8_t)));

[Søren Gjesse, 2012/11/15 08:11:30]

It would be nice if we could avoid the additional allocation here when dealing
with ASCII strings. How about optimistically allocating a CObject string for
ASCII assuming that it would most likely be ASCII. If we encounter a non-ASCII
character we can then start what the code currently does.

We could also add ReallocateDartCObjectString to extend the CObject string. This
is currently zone allocated, and will be the last object allocated in the zone.
The zone Realloc method could be optimized for expanding the last allocated
object.

Maybe that gets to complicated though - we can keep it as is for now.

[siva, 2012/11/15 22:42:59]

I agree that we can do some optimization here for pure
ASCII strings in a new CL.

On 2012/11/15 08:11:30, Søren Gjesse wrote:

+      intptr_t utf8_len = 0;
+      for (intptr_t i = 0; i < len; i++) {
+        latin1[i] = Read<uint8_t>();
+        utf8_len += Utf8::Length(latin1[i]);
+      }
+      Dart_CObject* object = AllocateDartCObjectString(utf8_len);
       AddBackRef(object_id, object, kIsDeserialized);
       char* p = object->value.as_string;
       for (intptr_t i = 0; i < len; i++) {
-        p[i] = Read<uint8_t>();
+        p += Utf8::Encode(latin1[i], p);
       }
-      p[len] = '\0';
+      *p = '\0';
+      ASSERT(p == (object->value.as_string + utf8_len));
+      ::free(latin1);
       return object;
     }
     case kTwoByteStringCid: {
@@ -376,7 +385,7 @@
         p += Utf8::Encode(utf16[i], p);
       }
       *p = '\0';
-      ASSERT(p == object->value.as_string + utf8_len);
+      ASSERT(p == (object->value.as_string + utf8_len));
       ::free(utf16);
       return object;
     }
@@ -772,16 +781,20 @@
       // Write out the serialization header value for this object.
       WriteInlinedHeader(object);
       // Write out the class and tags information.
-      WriteIndexedObject(type == Utf8::kAscii ? kOneByteStringCid
-                                              : kTwoByteStringCid);
+      WriteIndexedObject(type == Utf8::kLatin1 ? kOneByteStringCid
+                                               : kTwoByteStringCid);
       WriteIntptrValue(0);
       // Write string length, hash and content
       WriteSmi(len);
       WriteSmi(0);  // TODO(sgjesse): Hash - not written.
-      if (type == Utf8::kAscii) {
+      if (type == Utf8::kLatin1) {

[Søren Gjesse, 2012/11/15 08:11:30]

It would be nice if Utf8::CodePointCount could also still return kAscii if it
was all ASCII. Then we don't need to convert at all.

[siva, 2012/11/15 22:42:59]

Yes, this was an optimization I was planning on doing
might help regular VM code too. I would prefer doing it in
a new CL.

On 2012/11/15 08:11:30, Søren Gjesse wrote:

+        uint8_t* latin1_str =
+            reinterpret_cast<uint8_t*>(::malloc(len * sizeof(uint8_t)));
+        Utf8::DecodeToLatin1(utf8_str, utf8_len, latin1_str, len);

[Søren Gjesse, 2012/11/15 08:11:30]

If the Utf8 class had a function to convert a single code point we could avoid
allocating here, e.g.:

int i = 0;
while(i < len) {
  uint32_t code_point;
  i += Utf8::Decode(utf8_str + i, &code_point);
  ASSERT(code_point <= 255);
  Write<uint8_t>(code_point);
}

Utf8::DecodeCodePointToLatin1 returns the number of UTF-8 bytes consumed.
Alternatively the code point could be returned.

int i = 0;
while(i < len) {
  int consumed;
  Write<uint8_t>(Utf8::Decode(utf8_str + i, &consumed));
  ASSERT(consumed <= 2);
  i += consumed;
}

[siva, 2012/11/15 22:42:59]

Good point, would like to do this in a new CL in order to
avoid too many API changes in this CL.

On 2012/11/15 08:11:30, Søren Gjesse wrote:

         for (intptr_t i = 0; i < len; i++) {
-          Write<uint8_t>(utf8_str[i]);
+          Write<uint8_t>(latin1_str[i]);
         }
+        ::free(latin1_str);
       } else {
         // TODO(sgjesse): Make sure surrogate pairs are handled.
         uint16_t* utf16_str =