Thoughts on performance

[danielkutik]

Scenario:

We have following string:

<string name="greeting">Hello, {name}!</string>

What would happen if we would use Phrase like this ...

for(String name : listOfNames) {
  CharSequence greeting = Phrase.greeting()
    .name("GitHub user")
    .build(this);
}

... in a loop, let's say with 10,000 names. Every time we call greeting() a new instance of the class Phrase_greeting is constructed, that would leave us with a lot of classes to clean up afterwards. Wouldn't it make sense to keep the instance somehow?

[JakeWharton]

You could do it now.

Phrase_greeting greeting = Phrase.greeting();
for (String name : listOfNames) {
  CharSequence foo = greeting.name(name).build(this);
}

It'll still parse the string every iteration, though.

I didn't like taking the resources in the factory method but you'd have to in order to optimize this case.

[JakeWharton]

Ok what about this guy?

public final class Phrase {
  private static Phrase instance;

  public static Phrase from(Context context) {
    if (instance == null) {
      instance = new Phrase(context.getApplicationContext());
    }
    return instance;
  }

  private final Context context;
  private final SparseArray<ParsedTokensInSomeFormat> phrases = new SparseArray<>();
  private Locale currentLocale;

  private Phrase(Context context) {
    this.context = context;
    this.currentLocale = Locale.getDefault();
  }

  private ParsedTokensInSomeFormat tokensFor(int resId) {
    Locale defaultLocale = Locale.getDefault();
    if (currentLocale != defaultLocale) {
      phrases.clear();
      currentLocale = defaultLocale;
    }

    ParsedTokensInSomeFormat tokens = phrases.get(resId);
    if (tokens == null) {
      tokens = ParsedTokensInSomeFormat.from(context, resId);
      phrases.put(resId, tokens);
    }

    return tokens;
  }

  public Phrase_simple_test simple_test() {
    return new Phrase_simple_test(tokensFor(R.string.simple_test));
  }

  // ...
}

Usage:

Phrase.from(this).simple_test()
    .first_name("Jake")
    .last_name("Wharton")
    .build();

[JakeWharton]

Or:

public final class Phrase {
  private static final SparseArray<ParsedTokensInSomeFormat> TOKENIZED = new SparseArray<>();
  private static Locale currentLocale = Locale.getDefault();

  private static ParsedTokensInSomeFormat tokensFor(Context context, int resId) {
    Locale defaultLocale = Locale.getDefault();
    if (currentLocale != defaultLocale) {
      phrases.clear();
      currentLocale = defaultLocale;
    }

    ParsedTokensInSomeFormat tokens = TOKENIZED.get(resId);
    if (tokens == null) {
      tokens = ParsedTokensInSomeFormat.from(context, resId);
      TOKENIZED.put(resId, tokens);
    }

    return tokens;
  }

  public static Phrase_simple_test simple_test(Context context) {
    return new Phrase_simple_test(tokensFor(context, R.string.simple_test));
  }

  // ...
}

Usage:

Phrase.simple_test(this)
    .first_name("Jake")
    .last_name("Wharton")
    .build();

[danielkutik]

I like the second better... but...

Next scenario: We have a app that uses a big number of strings. SparseArray will become quite big.

Also be aware of the documentation of SparseArray:

The implementation is not intended to be appropriate for data structures that may contain large numbers of items. It is generally slower than a traditional HashMap, since lookups require a binary search and adds and removes require inserting and deleting entries in the array. For containers holding up to hundreds of items, the performance difference is not significant, less than 50%.

[JakeWharton]

Yeah it's implementation is a binary search tree packed into an int[] with adjacent T[] for the values. Lookup will be O(log n) since the R ids are alphabetically distributed and your application certainly isn't accessing them in alphabetical order. Additionally, total strings don't matter, only the number of format strings you have. Do you use more than hundreds of format strings per app process? Even then, the instances returned from calling the string methods (e.g., simple_test) are perfectly re-usable on a single thread for your tight-loop scenario.

[danielkutik]

I know I'm nit-picking :) Any thoughts on android.support.v4.util.LruCache?

[JakeWharton]

I thought about it. You pay the boxing penalty for integers and suffer the overhead of all the Map.Entry instances.

I'd rather just go with SparseArray for simplicity and then benchmark actual usage later.