Improve the DecimalFormat JNI.

We don't need two identical copies of the code for double and long; ICU uses
overloading, and we should take advantage of that. We can also improve the code
to remove unnecessary heap allocation, remove unnecessary temporary copies, and
only make JNI calls and ask for the attribute data when necessary.

I've also switched the code from the thread-unsafe strtok(3) to strtok_r(3).

I've also removed unnecessary temporary char[]s and copying in DecimalFormat.

I've also fixed another instance of the "if (doubleValue == longValue) longPath"
anti-pattern that gets -0.0 wrong. (It's also worth noting that caliper says
the difference between the double and long paths is very small, on the order
of 2us.)

(The new code takes about 20us per call compared to 60us for the old code,
measured on passion-eng.)

2 files changed, 67 insertions, 213 deletions

diff --git a/icu/src/main/java/com/ibm/icu4jni/text/DecimalFormat.java b/icu/src/main/java/com/ibm/icu4jni/text/DecimalFormat.java
index df76385..4b296d5 100644
--- a/icu/src/main/java/com/ibm/icu4jni/text/DecimalFormat.java
+++ b/icu/src/main/java/com/ibm/icu4jni/text/DecimalFormat.java
@@ -176,8 +176,7 @@ public class DecimalFormat extends NumberFormat {
             throw new NullPointerException();
         }
         String fieldType = getFieldType(field.getFieldAttribute());
-        String result = NativeDecimalFormat.format(this.addr, value, field, fieldType, null);
-        buffer.append(result.toCharArray(), 0, result.length());
+        buffer.append(NativeDecimalFormat.format(this.addr, value, field, fieldType, null));
         return buffer;
     }
 
@@ -187,8 +186,7 @@ public class DecimalFormat extends NumberFormat {
             throw new NullPointerException();
         }
         String fieldType = getFieldType(field.getFieldAttribute());
-        String result = NativeDecimalFormat.format(this.addr, value, field, fieldType, null);
-        buffer.append(result.toCharArray(), 0, result.length());
+        buffer.append(NativeDecimalFormat.format(this.addr, value, field, fieldType, null));
         return buffer;
     }
 
@@ -240,19 +238,15 @@ public class DecimalFormat extends NumberFormat {
             scale = makeScalePositive(scale, val);
             text = NativeDecimalFormat.format(this.addr, val.toString(), null,
                     null, attributes, scale);
-        } else {
+        } else if (number instanceof Double || number instanceof Float) {
             double dv = number.doubleValue();
+            text = NativeDecimalFormat.format(this.addr, dv, null, null, attributes);
+        } else {
             long lv = number.longValue();
-            if (dv == lv) {
-                text = NativeDecimalFormat.format(this.addr, lv, null,
-                        null, attributes);
-            } else {
-                text = NativeDecimalFormat.format(this.addr, dv, null,
-                        null, attributes);
-            }
+            text = NativeDecimalFormat.format(this.addr, lv, null, null, attributes);
         }
 
-        AttributedString as = new AttributedString(text.toString());
+        AttributedString as = new AttributedString(text);
 
         String[] attrs = attributes.toString().split(";");
         // add NumberFormat field attributes to the AttributedString
diff --git a/icu/src/main/native/DecimalFormatInterface.cpp b/icu/src/main/native/DecimalFormatInterface.cpp
index 0b19716..caae492 100644
--- a/icu/src/main/native/DecimalFormatInterface.cpp
+++ b/icu/src/main/native/DecimalFormatInterface.cpp
@@ -1,18 +1,17 @@
 /*
- * Copyright 2006 The Android Open Source Project
+ * Copyright (C) 2006 The Android Open Source Project
  *
- * Internal native functions.  All of the functions defined here make
- * direct use of VM functions or data structures, so they can't be written
- * with JNI and shouldn't really be in a shared library.
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
  *
- * All functions here either complete quickly or are used to enter a wait
- * state, so we don't set the thread status to THREAD_NATIVE when executing
- * these methods.  This means that the GC will wait for these functions
- * to finish.  DO NOT perform long operations or blocking I/O in here.
+ *      http://www.apache.org/licenses/LICENSE-2.0
  *
- * In some cases we're following the division of labor defined by GNU
- * ClassPath, e.g. java.lang.Thread has "Thread" and "VMThread", with
- * the VM-specific behavior isolated in VMThread.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
  */
 
 #define LOG_TAG "DecimalFormatInterface"
@@ -254,221 +253,82 @@ static jstring toPatternImpl(JNIEnv *env, jclass clazz, jint addr,
     return res;
 }
 
-static jstring formatLong(JNIEnv *env, jclass clazz, jint addr, jlong value,
-        jobject field, jstring fieldType, jobject attributes) {
-
-    const char * fieldPositionClassName = "java/text/FieldPosition";
-    const char * stringBufferClassName = "java/lang/StringBuffer";
-    jclass fieldPositionClass = env->FindClass(fieldPositionClassName);
-    jclass stringBufferClass = env->FindClass(stringBufferClassName);
-    jmethodID setBeginIndexMethodID = env->GetMethodID(fieldPositionClass,
-            "setBeginIndex", "(I)V");
-    jmethodID setEndIndexMethodID = env->GetMethodID(fieldPositionClass,
-            "setEndIndex", "(I)V");
-    jmethodID appendMethodID = env->GetMethodID(stringBufferClass,
-            "append", "(Ljava/lang/String;)Ljava/lang/StringBuffer;");
-
-    const char * fieldName = NULL;
+template <typename T>
+static jstring format(JNIEnv *env, jint addr, jobject field, jstring fieldType, jobject attributes, T val) {
+    UErrorCode status = U_ZERO_ERROR;
 
-    if(fieldType != NULL) {
-        fieldName = env->GetStringUTFChars(fieldType, NULL);
+    DecimalFormat::AttributeBuffer attrBuffer;
+    attrBuffer.buffer = NULL;
+    DecimalFormat::AttributeBuffer* attrBufferPtr = NULL;
+    if (attributes != NULL || (fieldType != NULL && field != NULL)) {
+        attrBufferPtr = &attrBuffer;
+        // ICU requires that this is dynamically allocated and non-zero size.
+        // ICU grows it in chunks of 128 bytes, so that's a reasonable initial size.
+        attrBuffer.bufferSize = 128;
+        attrBuffer.buffer = new char[attrBuffer.bufferSize];
+        attrBuffer.buffer[0] = '\0';
     }
 
-    uint32_t reslenneeded;
-    int64_t val = value;
-    UChar *result = NULL;
-
     FieldPosition fp;
     fp.setField(FieldPosition::DONT_CARE);
 
-    UErrorCode status = U_ZERO_ERROR;
-
-    DecimalFormat::AttrBuffer attrBuffer = NULL;
-    attrBuffer = (DecimalFormat::AttrBuffer) malloc(sizeof(*attrBuffer));
-    attrBuffer->bufferSize = 128;
-    attrBuffer->buffer = (char *) malloc(129 * sizeof(char));
-    attrBuffer->buffer[0] = '\0';
-
-    DecimalFormat *fmt = (DecimalFormat *)(int)addr;
-
-    UnicodeString *res = new UnicodeString();
-
-    fmt->format(val, *res, fp, attrBuffer);
-
-    reslenneeded = res->extract(NULL, 0, status);
-
-    if(status==U_BUFFER_OVERFLOW_ERROR) {
-        status=U_ZERO_ERROR;
-
-        result = (UChar*)malloc(sizeof(UChar) * (reslenneeded + 1));
-
-        res->extract(result, reslenneeded + 1, status);
-    }
-    if (icu4jni_error(env, status) != FALSE) {
-        free(attrBuffer->buffer);
-        free(attrBuffer);
-        free(result);
-        delete(res);
-        return NULL;
-    }
-
-    int attrLength = 0;
-
-    attrLength = (strlen(attrBuffer->buffer) + 1 );
-
-    if(strlen(attrBuffer->buffer) > 0) {
+    UnicodeString str;
+    DecimalFormat* fmt = reinterpret_cast<DecimalFormat*>(static_cast<uintptr_t>(addr));
+    fmt->format(val, str, fp, attrBufferPtr);
 
+    if (attrBufferPtr && strlen(attrBuffer.buffer) > 0) {
         // check if we want to get all attributes
-        if(attributes != NULL) {
-            jstring attrString = env->NewStringUTF(attrBuffer->buffer + 1);  // cut off the leading ';'
+        if (attributes != NULL) {
+            jstring attrString = env->NewStringUTF(attrBuffer.buffer + 1);  // cut off the leading ';'
+            jclass stringBufferClass = env->FindClass("java/lang/StringBuffer");
+            jmethodID appendMethodID = env->GetMethodID(stringBufferClass, "append", "(Ljava/lang/String;)Ljava/lang/StringBuffer;");
             env->CallObjectMethod(attributes, appendMethodID, attrString);
         }
 
         // check if we want one special attribute returned in the given FieldPos
-        if(fieldName != NULL && field != NULL) {
-            const char *delimiter = ";";
-            int begin;
-            int end;
-            char * resattr;
-            resattr = strtok(attrBuffer->buffer, delimiter);
+        if (fieldType != NULL && field != NULL) {
+            const char* fieldName = env->GetStringUTFChars(fieldType, NULL);
 
-            while(resattr != NULL && strcmp(resattr, fieldName) != 0) {
-                resattr = strtok(NULL, delimiter);
+            const char* delimiter = ";";
+            char* context = NULL;
+            char* resattr = strtok_r(attrBuffer.buffer, delimiter, &context);
+
+            while (resattr != NULL && strcmp(resattr, fieldName) != 0) {
+                resattr = strtok_r(NULL, delimiter, &context);
             }
 
-            if(resattr != NULL && strcmp(resattr, fieldName) == 0) {
-                resattr = strtok(NULL, delimiter);
-                begin = (int) strtol(resattr, NULL, 10);
-                resattr = strtok(NULL, delimiter);
-                end = (int) strtol(resattr, NULL, 10);
+            if (resattr != NULL && strcmp(resattr, fieldName) == 0) {
+                resattr = strtok_r(NULL, delimiter, &context);
+                int begin = (int) strtol(resattr, NULL, 10);
+                resattr = strtok_r(NULL, delimiter, &context);
+                int end = (int) strtol(resattr, NULL, 10);
 
+                jclass fieldPositionClass = env->FindClass("java/text/FieldPosition");
+                jmethodID setBeginIndexMethodID = env->GetMethodID(fieldPositionClass, "setBeginIndex", "(I)V");
+                jmethodID setEndIndexMethodID = env->GetMethodID(fieldPositionClass, "setEndIndex", "(I)V");
                 env->CallVoidMethod(field, setBeginIndexMethodID, (jint) begin);
                 env->CallVoidMethod(field, setEndIndexMethodID, (jint) end);
             }
+            env->ReleaseStringUTFChars(fieldType, fieldName);
         }
     }
 
-    if(fieldType != NULL) {
-        env->ReleaseStringUTFChars(fieldType, fieldName);
-    }
-
-    jstring resulting = env->NewString(result, reslenneeded);
-
-    free(attrBuffer->buffer);
-    free(attrBuffer);
-    free(result);
-    delete(res);
-
-    return resulting;
+    const UChar* chars = str.getBuffer();
+    jstring result = env->NewString(chars, str.length());
+    delete[] attrBuffer.buffer;
+    return result;
 }
 
-static jstring formatDouble(JNIEnv *env, jclass clazz, jint addr, jdouble value,
+static jstring formatLong(JNIEnv* env, jclass, jint addr, jlong value,
         jobject field, jstring fieldType, jobject attributes) {
+    int64_t longValue = value;
+    return format(env, addr, field, fieldType, attributes, longValue);
+}
 
-    const char * fieldPositionClassName = "java/text/FieldPosition";
-    const char * stringBufferClassName = "java/lang/StringBuffer";
-    jclass fieldPositionClass = env->FindClass(fieldPositionClassName);
-    jclass stringBufferClass = env->FindClass(stringBufferClassName);
-    jmethodID setBeginIndexMethodID = env->GetMethodID(fieldPositionClass,
-            "setBeginIndex", "(I)V");
-    jmethodID setEndIndexMethodID = env->GetMethodID(fieldPositionClass,
-            "setEndIndex", "(I)V");
-    jmethodID appendMethodID = env->GetMethodID(stringBufferClass,
-            "append", "(Ljava/lang/String;)Ljava/lang/StringBuffer;");
-
-    const char * fieldName = NULL;
-
-    if(fieldType != NULL) {
-        fieldName = env->GetStringUTFChars(fieldType, NULL);
-    }
-
-    uint32_t reslenneeded;
-    double val = value;
-    UChar *result = NULL;
-
-    FieldPosition fp;
-    fp.setField(FieldPosition::DONT_CARE);
-
-    UErrorCode status = U_ZERO_ERROR;
-
-    DecimalFormat::AttrBuffer attrBuffer = NULL;
-    attrBuffer = (DecimalFormat::AttrBuffer) malloc(sizeof(*attrBuffer));
-    attrBuffer->bufferSize = 128;
-    attrBuffer->buffer = (char *) malloc(129 * sizeof(char));
-    attrBuffer->buffer[0] = '\0';
-
-    DecimalFormat *fmt = (DecimalFormat *)(int)addr;
-
-    UnicodeString *res = new UnicodeString();
-
-    fmt->format(val, *res, fp, attrBuffer);
-
-    reslenneeded = res->extract(NULL, 0, status);
-
-    if(status==U_BUFFER_OVERFLOW_ERROR) {
-        status=U_ZERO_ERROR;
-
-        result = (UChar*)malloc(sizeof(UChar) * (reslenneeded + 1));
-
-        res->extract(result, reslenneeded + 1, status);
-
-    }
-    if (icu4jni_error(env, status) != FALSE) {
-        free(attrBuffer->buffer);
-        free(attrBuffer);
-        free(result);
-        delete(res);
-        return NULL;
-    }
-
-    int attrLength = 0;
-
-    attrLength = (strlen(attrBuffer->buffer) + 1 );
-
-    if(strlen(attrBuffer->buffer) > 0) {
-
-        // check if we want to get all attributes
-        if(attributes != NULL) {
-            jstring attrString = env->NewStringUTF(attrBuffer->buffer + 1);  // cut off the leading ';'
-            env->CallObjectMethod(attributes, appendMethodID, attrString);
-        }
-
-        // check if we want one special attribute returned in the given FieldPos
-        if(fieldName != NULL && field != NULL) {
-            const char *delimiter = ";";
-            int begin;
-            int end;
-            char * resattr;
-            resattr = strtok(attrBuffer->buffer, delimiter);
-
-            while(resattr != NULL && strcmp(resattr, fieldName) != 0) {
-                resattr = strtok(NULL, delimiter);
-            }
-
-            if(resattr != NULL && strcmp(resattr, fieldName) == 0) {
-                resattr = strtok(NULL, delimiter);
-                begin = (int) strtol(resattr, NULL, 10);
-                resattr = strtok(NULL, delimiter);
-                end = (int) strtol(resattr, NULL, 10);
-
-                env->CallVoidMethod(field, setBeginIndexMethodID, (jint) begin);
-                env->CallVoidMethod(field, setEndIndexMethodID, (jint) end);
-            }
-        }
-    }
-
-    if(fieldType != NULL) {
-        env->ReleaseStringUTFChars(fieldType, fieldName);
-    }
-
-    jstring resulting = env->NewString(result, reslenneeded);
-
-    free(attrBuffer->buffer);
-    free(attrBuffer);
-    free(result);
-    delete(res);
-
-    return resulting;
+static jstring formatDouble(JNIEnv* env, jclass, jint addr, jdouble value,
+        jobject field, jstring fieldType, jobject attributes) {
+    double doubleValue = value;
+    return format(env, addr, field, fieldType, attributes, doubleValue);
 }
 
 static jstring formatDigitList(JNIEnv *env, jclass clazz, jint addr, jstring value,