Experimental CL on top of https://codereview.chromium.org/1812673005/

src/runtime/runtime-i18n.cc

Index: src/runtime/runtime-i18n.cc
diff --git a/src/runtime/runtime-i18n.cc b/src/runtime/runtime-i18n.cc
index 8f05809c50c7c87007fb27d2b18d93b6299edd60..cb863f65534bdcaff66e40b518bb1179261a4aaf 100644
--- a/src/runtime/runtime-i18n.cc
+++ b/src/runtime/runtime-i18n.cc
@@ -753,6 +753,7 @@ RUNTIME_FUNCTION(Runtime_BreakIteratorBreakType) {
 }
 
 namespace {
+#if 0
 inline void LocaleConvertCaseHelper(icu::UnicodeString* s, bool is_to_upper,
                                     const icu::Locale& locale) {
   if (is_to_upper)
@@ -760,6 +761,7 @@ inline void LocaleConvertCaseHelper(icu::UnicodeString* s, bool is_to_upper,
   else
     s->toLower(locale);
 }
+#endif
 void ConvertCaseWithTransliterator(icu::UnicodeString* input,
                                    const char* transliterator_id) {
   UErrorCode status = U_ZERO_ERROR;
@@ -779,42 +781,52 @@ MUST_USE_RESULT Object* LocaleConvertCase(Handle<String> s, Isolate* isolate,
   RUNTIME_ASSERT(locale_id >= -1 &&
                  locale_id < static_cast<int>(arraysize(conversion_locales)));
   int32_t length = s->length();
-  icu::UnicodeString converted;
+  base::SmartArrayPointer<uc16> sap;
+  const UChar* src;
+  Handle<SeqTwoByteString> result =
+      isolate->factory()->NewRawTwoByteString(length).ToHandleChecked();
   {
     DisallowHeapAllocation no_gc;
     DCHECK(s->IsFlat());
     String::FlatContent flat = s->GetFlatContent();
-
-    const UChar* src;
     if (flat.IsOneByte()) {
-      base::SmartArrayPointer<uc16> sap = s->ToWideCString();
+      sap = s->ToWideCString();
       src = reinterpret_cast<const UChar*>(sap.get());
-      converted = icu::UnicodeString(src, length);
     } else {
+      // Will the memory pointed to by |src| be available reliably
+      // outside the block? Thai, will it survive 'gc' that may
+      // be invoked when |result| is "relloced" below?
       src = reinterpret_cast<const UChar*>(flat.ToUC16Vector().start());

[jungshik at Google, 2016/04/14 00:32:34]

adamk@ and littledan@:

Can you answer my question in the comment above? 

SeqTwoByteString was created outside the block (before the block with
DisallowHeapAllocation). However, the length of |result| (SetTwoByteString) is
adjusted by calling |set_length|. Would this affect whether or not the memory
pointed to by |src|. FlatContent just points to the buffer of a flattended
string, |s| in this case which is passed from a caller. However, "realloc"(? :
length adjustment) would trigger gc and make |s| go away along with the buffer
pointed to bey |src|?  

With a debug build when I run test262/intl402/String/*, I got a crash (not in
this file but at the end of a process clean-up). 

Program received signal SIGSEGV, Segmentation fault.
0x000000000041e30c in v8::base::NoBarrier_Load (ptr=0xffffffff)
    at ../../src/base/atomicops_internals_x86_gcc.h:236
236       return *ptr;
(gdb) bt
#0  0x000000000041e30c in v8::base::NoBarrier_Load (ptr=0xffffffff)
    at ../../src/base/atomicops_internals_x86_gcc.h:236
#1  0x000000000041e279 in v8::internal::HeapObject::map_word (this=0x100000000)
    at ../../src/objects-inl.h:1351
#2  0x000000000041dc25 in v8::internal::HeapObject::map (this=0x100000000)
    at ../../src/objects-inl.h:1305
#3  0x000000000041e1f5 in v8::internal::HeapObject::IsMap (this=0x100000000)
    at ../../src/objects-inl.h:677
#4  0x00000000008f8574 in v8::internal::NewSpace::Verify (this=0x20f0dc0)
    at ../../src/heap/spaces.cc:1618
#5  0x0000000000872154 in v8::internal::Heap::Verify (this=0x20f0220)
    at ../../src/heap/heap.cc:4625
#6  0x0000000000894e5e in v8::internal::Heap::TearDown (this=0x20f0220)
    at ../../src/heap/heap.cc:5384
#7  0x00000000009550aa in v8::internal::Isolate::Deinit (this=0x20f0200)
    at ../../src/isolate.cc:1980
#8  0x0000000000954d19 in v8::internal::Isolate::TearDown (this=0x20f0200)
    at ../../src/isolate.cc:1905
#9  0x000000000043b62d in v8::Isolate::Dispose (this=0x20f0200)
    at ../../src/api.cc:7315
#10 0x000000000041684a in v8::Shell::Main (argc=17, argv=0x7fffffffd648)
    at ../../src/d8.cc:2503
#11 0x000000000041cf72 in main (argc=17, argv=0x7fffffffd648)
    at ../../src/d8.cc:2516

[adamk, 2016/04/14 01:56:35]

What you're doing here is basically defeating the point of putting
DisallowHeapAllocation in this block: you can't rely on flat (or things vended
from it) being valid if a heap allocation has happened since GetFlatContent()
was called.

But I don't see any V8 heap allocation below, so it's possible you can fix this
just by widening the contents of the DisallowHeapAllocation block?

-      converted = icu::UnicodeString(src, length);
     }
   }
 
-  if (locale_id == -1) {
-    LocaleConvertCaseHelper(&converted, is_to_upper, icu::Locale::getRoot());
-  } else if (V8_UNLIKELY(locale_id == 1 && is_to_upper)) {
-    // TODO(jshin): Once http://bugs.icu-project.org/trac/ticket/10582 is
-    // fixed, remove this special-casing for uppercasing in Greek(el) locale.
-    // This is ~500 times slower than using the case conversion API.
-    ConvertCaseWithTransliterator(&converted, "el-Upper");
-  } else {
-    LocaleConvertCaseHelper(&converted, is_to_upper,
-                            icu::Locale(conversion_locales[locale_id]));
+  if (V8_LIKELY(locale_id != 1 || !is_to_upper)) {
+    typedef int32_t (*case_conversion_fn)(
+        UChar * dest, int32_t destCapacity, const UChar* src, int32_t srcLength,
+        const char* locale, UErrorCode* pErrorCode);
+    case_conversion_fn fn = is_to_upper ? u_strToUpper : u_strToLower;
+
+    int32_t target_length = length;
+    const char* locale = locale_id == -1 ? "" : conversion_locales[locale_id];
+    UErrorCode error;
+    do {
+      error = U_ZERO_ERROR;
+      target_length = fn(reinterpret_cast<UChar*>(result->GetChars()),
+                         target_length, src, length, locale, &error);
+      result->set_length(target_length);
+    } while (error == U_BUFFER_OVERFLOW_ERROR);
+    return U_SUCCESS(error) ? *result : *s;
   }
 
-  Handle<String> result;
+  icu::UnicodeString converted(false, src, length);
+  ConvertCaseWithTransliterator(&converted, "el-Upper");
+  Handle<String> greek_result;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-      isolate, result,
+      isolate, greek_result,
       isolate->factory()->NewStringFromTwoByte(Vector<const uint16_t>(
           reinterpret_cast<const uint16_t*>(converted.getBuffer()),
           converted.length())));
-  return *result;
+  return *greek_result;
 }
 
 inline bool IsASCIIUpper(uint16_t ch) { return ch >= 'A' && ch <= 'Z'; }