Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid String concatenation for lookup in StaticMessageSource #22451

Closed

Conversation

@stsypanov
Copy link
Contributor

stsypanov commented Feb 21, 2019

Currently StaticMessageSource employs String composed of key and stringified Locale as key in map:

public class StaticMessageSource extends AbstractMessageSource {

	/** Map from 'code + locale' keys to message Strings. */
	private final Map<String, String> messages = new HashMap<>();

	private final Map<String, MessageFormat> cachedMessageFormats = new HashMap<>();

        //...
}

As a result at each call to Map::get/Map::put we have to concat Strings resulting in allocations of StringBuilder and copying char[]. Instead I propose to use separately defined immutable Key.

I've tested behaviour with benchmark

@State(Scope.Thread)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class LocaleToStringBenchmark {

  @Benchmark
  public Object concatenation(Data data) {
    return data.code + '_' + data.locale;
  }

  @Benchmark
  public Object compositeKey(Data data) {
    return new Key(data.code, data.locale);
  }

  @Benchmark
  public int hashCodeCompositeString(Data data) {
    return data.compositeString.hashCode();
  }

  @Benchmark
  public int hashCodeCompositeKey(Data data) {
    return data.compositeKey.hashCode();
  }

  @Benchmark
  public boolean equalsCompositeString(Data data) {
    return data.compositeString.equals(data.anotherCompositeString);
  }

  @Benchmark
  public boolean equalsCompositeKey(Data data) {
    return data.compositeKey.equals(data.anotherCompositeKey);
  }

  @State(Scope.Thread)
  public static class Data {
    private final String code = "code1";
    private final Locale locale = Locale.getDefault();

    private final Key compositeKey = new Key(code, locale);
    private final String compositeString = code + '_' + locale;

    private final Key anotherCompositeKey = new Key(code, locale);
    private final String anotherCompositeString = code + '_' + locale;
  }

  private static final class Key {
    private final String code;
    private final Locale locale;

    private Key(String code, Locale locale) {
      this.code = code;
      this.locale = locale;
    }

    @Override
    public boolean equals(Object o) {
      if (this == o) return true;
      if (o == null || getClass() != o.getClass()) return false;

      Key key = (Key) o;

      if (!code.equals(key.code)) return false;
      return locale.equals(key.locale);
    }

    @Override
    public int hashCode() {
      return 31 * code.hashCode() + locale.hashCode();
    }
  }
}

Patched consumes much less memory on key allocation and does it faster, performance of equals/hashCode is mostly the same (tested at work on i7-7700):

JDK 8
                                             Mode  Cnt     Score     Error   Units

compositeKey                                 avgt   25     5,134 ±   0,274   ns/op
concatenation                                avgt   25    67,999 ±   5,872   ns/op

compositeKey:·gc.alloc.rate.norm             avgt   25    24,000 ±   0,001    B/op
concatenation:·gc.alloc.rate.norm            avgt   25   224,000 ±   0,001    B/op

equalsCompositeKey                           avgt   25     3,229 ±   0,161   ns/op
equalsCompositeString                        avgt   25     6,447 ±   0,448   ns/op

hashCodeCompositeKey                         avgt   25     2,584 ±   0,054   ns/op
hashCodeCompositeString                      avgt   25     2,069 ±   0,028   ns/op

JDK 11
                                             Mode  Cnt     Score    Error   Units

compositeKey                                 avgt   25     5,851 ±  0,271   ns/op
concatenation                                avgt   25    61,900 ±  4,406   ns/op

compositeKey:·gc.alloc.rate.norm             avgt   25    24,000 ±  0,001    B/op
concatenation:·gc.alloc.rate.norm            avgt   25   168,000 ±  0,001    B/op

equalsCompositeKey                           avgt   25     3,177 ±  0,254   ns/op
equalsCompositeString                        avgt   25     5,787 ±  0,305   ns/op

hashCodeCompositeKey                         avgt   25     2,947 ±  0,105   ns/op
hashCodeCompositeString                      avgt   25     2,187 ±  0,083   ns/op
@jhoeller jhoeller self-assigned this Feb 21, 2019
@jhoeller jhoeller modified the milestones: 5.2 M1, 5.2 M2 Feb 21, 2019
@jhoeller jhoeller added the in: core label Mar 27, 2019
@jhoeller jhoeller modified the milestones: 5.2 M2, 5.x Backlog Mar 27, 2019
@stsypanov stsypanov force-pushed the stsypanov:use-composite-key branch from 68eb1d6 to 3a3058b Aug 16, 2019
@stsypanov stsypanov changed the title Use separately defined Key as composite key in StaticMessageSource Use Arrays.asList as composite key in StaticMessageSource Aug 21, 2019
@stsypanov

This comment has been minimized.

Copy link
Contributor Author

stsypanov commented Aug 21, 2019

@jhoeller I've slightly simplified the patch to use Array.asList

@stsypanov stsypanov force-pushed the stsypanov:use-composite-key branch from 3a3058b to e4241fd Nov 13, 2019
@stsypanov

This comment has been minimized.

Copy link
Contributor Author

stsypanov commented Nov 13, 2019

@jhoeller hello, any updates on this?

@jhoeller jhoeller modified the milestones: 5.x Backlog, 5.2.2 Nov 13, 2019
@jhoeller

This comment has been minimized.

Copy link
Contributor

jhoeller commented Nov 13, 2019

On review, I'll rather go with a nested Map<String, Map<Locale, ...>> structure here, similar to ReloadableResourceBundleMessageSource. We can even have a single structure with a message holder that contains both the raw message and a lazily resolved MessageFormat instance.

As a side note, part of the original design of StaticMessageSource was to have a human-readable map representation in its toString() output. Rendering a flat map with its concatenated String keys was a nice benefit there; fortunately, even the nested map structure looks alright there now.

I'll repurpose this issue accordingly. Thanks for the PR, in any case!

@jhoeller jhoeller changed the title Use Arrays.asList as composite key in StaticMessageSource Avoid String concatenation for lookup in StaticMessageSource Nov 13, 2019
@jhoeller jhoeller closed this in 3dc5e7b Nov 13, 2019
@stsypanov stsypanov deleted the stsypanov:use-composite-key branch Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.