diff --git a/java/libphonenumber/src/com/google/i18n/phonenumbers/MetadataLoader.java b/java/libphonenumber/src/com/google/i18n/phonenumbers/MetadataLoader.java index 1ab3e9317..9904bcdaa 100644 --- a/java/libphonenumber/src/com/google/i18n/phonenumbers/MetadataLoader.java +++ b/java/libphonenumber/src/com/google/i18n/phonenumbers/MetadataLoader.java @@ -23,7 +23,8 @@ import java.io.InputStream; */ public interface MetadataLoader { /** - * Returns an input stream corresponding to the metadata to load. + * Returns an input stream corresponding to the metadata to load. This method may be called + * concurrently so implementations must be thread-safe. * * @param metadataFileName File name (including path) of metadata to load. File path is an * absolute class path like /com/google/i18n/phonenumbers/data/PhoneNumberMetadataProto. diff --git a/java/libphonenumber/src/com/google/i18n/phonenumbers/MultiFileMetadataSourceImpl.java b/java/libphonenumber/src/com/google/i18n/phonenumbers/MultiFileMetadataSourceImpl.java index 4f6b29da0..37d7bca6a 100644 --- a/java/libphonenumber/src/com/google/i18n/phonenumbers/MultiFileMetadataSourceImpl.java +++ b/java/libphonenumber/src/com/google/i18n/phonenumbers/MultiFileMetadataSourceImpl.java @@ -22,10 +22,8 @@ import com.google.i18n.phonenumbers.nano.Phonemetadata.PhoneMetadataCollection; import java.io.IOException; import java.io.InputStream; import java.io.ObjectInputStream; -import java.util.Collections; -import java.util.HashMap; +import java.util.concurrent.ConcurrentHashMap; import java.util.List; -import java.util.Map; import java.util.logging.Level; import java.util.logging.Logger; @@ -41,18 +39,14 @@ final class MultiFileMetadataSourceImpl implements MetadataSource { "/com/google/i18n/phonenumbers/data/PhoneNumberMetadataProto"; // A mapping from a region code to the PhoneMetadata for that region. - // Note: Synchronization, though only needed for the Android version of the library, is used in - // all versions for consistency. - private final Map regionToMetadataMap = - Collections.synchronizedMap(new HashMap()); + private final ConcurrentHashMap geographicalRegions = + new ConcurrentHashMap(); // A mapping from a country calling code for a non-geographical entity to the PhoneMetadata for // that country calling code. Examples of the country calling codes include 800 (International // Toll Free Service) and 808 (International Shared Cost Service). - // Note: Synchronization, though only needed for the Android version of the library, is used in - // all versions for consistency. - private final Map countryCodeToNonGeographicalMetadataMap = - Collections.synchronizedMap(new HashMap()); + private final ConcurrentHashMap nonGeographicalRegions = + new ConcurrentHashMap(); // The prefix of the metadata files from which region data is loaded. private final String filePrefix; @@ -60,97 +54,97 @@ final class MultiFileMetadataSourceImpl implements MetadataSource { // The metadata loader used to inject alternative metadata sources. private final MetadataLoader metadataLoader; - // It is assumed that metadataLoader is not null. - public MultiFileMetadataSourceImpl(String filePrefix, MetadataLoader metadataLoader) { + // It is assumed that metadataLoader is not null. If needed, checks should happen before passing + // here. + // @VisibleForTesting + MultiFileMetadataSourceImpl(String filePrefix, MetadataLoader metadataLoader) { this.filePrefix = filePrefix; this.metadataLoader = metadataLoader; } - // It is assumed that metadataLoader is not null. + // It is assumed that metadataLoader is not null. If needed, checks should happen before passing + // here. public MultiFileMetadataSourceImpl(MetadataLoader metadataLoader) { this(META_DATA_FILE_PREFIX, metadataLoader); } @Override public PhoneMetadata getMetadataForRegion(String regionCode) { - synchronized (regionToMetadataMap) { - if (!regionToMetadataMap.containsKey(regionCode)) { - // The regionCode here will be valid and won't be '001', so we don't need to worry about - // what to pass in for the country calling code. - loadMetadataFromFile(regionCode, 0); - } - } - return regionToMetadataMap.get(regionCode); + PhoneMetadata metadata = geographicalRegions.get(regionCode); + return (metadata != null) ? metadata : loadMetadataFromFile( + regionCode, geographicalRegions, filePrefix, metadataLoader); } @Override public PhoneMetadata getMetadataForNonGeographicalRegion(int countryCallingCode) { - synchronized (countryCodeToNonGeographicalMetadataMap) { - if (!countryCodeToNonGeographicalMetadataMap.containsKey(countryCallingCode)) { - List regionCodes = - CountryCodeToRegionCodeMap.getCountryCodeToRegionCodeMap().get(countryCallingCode); - // We can assume that if the country calling code maps to the non-geo entity region code - // then that's the only region code it maps to. - if (regionCodes.size() == 1 && - PhoneNumberUtil.REGION_CODE_FOR_NON_GEO_ENTITY.equals(regionCodes.get(0))) { - loadMetadataFromFile(PhoneNumberUtil.REGION_CODE_FOR_NON_GEO_ENTITY, countryCallingCode); - } - } + PhoneMetadata metadata = nonGeographicalRegions.get(countryCallingCode); + if (metadata != null) { + return metadata; } - return countryCodeToNonGeographicalMetadataMap.get(countryCallingCode); + if (isNonGeographical(countryCallingCode)) { + return loadMetadataFromFile( + countryCallingCode, nonGeographicalRegions, filePrefix, metadataLoader); + } + // The given country calling code was for a geographical region. + return null; + } + + // A country calling code is non-geographical if it only maps to the non-geographical region code, + // i.e. "001". + private boolean isNonGeographical(int countryCallingCode) { + List regionCodes = + CountryCodeToRegionCodeMap.getCountryCodeToRegionCodeMap().get(countryCallingCode); + return (regionCodes.size() == 1 + && PhoneNumberUtil.REGION_CODE_FOR_NON_GEO_ENTITY.equals(regionCodes.get(0))); } + /** + * @param key The geographical region code or the non-geographical region's country + * calling code. + * @param map The map to contain the mapping from {@code key} to the corresponding + * metadata. + * @param filePrefix The prefix of the metadata files from which region data is loaded. + * @param metadataLoader The metadata loader used to inject alternative metadata sources. + */ // @VisibleForTesting - void loadMetadataFromFile(String regionCode, int countryCallingCode) { - boolean isNonGeoRegion = PhoneNumberUtil.REGION_CODE_FOR_NON_GEO_ENTITY.equals(regionCode); - String fileName = filePrefix + "_" + - (isNonGeoRegion ? String.valueOf(countryCallingCode) : regionCode); + static PhoneMetadata loadMetadataFromFile( + T key, ConcurrentHashMap map, String filePrefix, + MetadataLoader metadataLoader) { + // We assume key.toString() is well-defined. + String fileName = filePrefix + "_" + key; InputStream source = metadataLoader.loadMetadata(fileName); if (source == null) { - logger.log(Level.SEVERE, "missing metadata: " + fileName); throw new IllegalStateException("missing metadata: " + fileName); } - try { - PhoneMetadataCollection metadataCollection = - loadMetadataAndCloseInput(new ObjectInputStream(source)); - PhoneMetadata[] metadataList = metadataCollection.metadata; - if (metadataList.length == 0) { - logger.log(Level.SEVERE, "empty metadata: " + fileName); - throw new IllegalStateException("empty metadata: " + fileName); - } - if (metadataList.length > 1) { - logger.log(Level.WARNING, "invalid metadata (too many entries): " + fileName); - } - PhoneMetadata metadata = metadataList[0]; - if (isNonGeoRegion) { - countryCodeToNonGeographicalMetadataMap.put(countryCallingCode, metadata); - } else { - regionToMetadataMap.put(regionCode, metadata); - } - } catch (IOException e) { - logger.log(Level.SEVERE, "cannot load/parse metadata: " + fileName, e); - throw new RuntimeException("cannot load/parse metadata: " + fileName, e); + PhoneMetadataCollection metadataCollection = loadMetadataAndCloseInput(source); + PhoneMetadata[] metadataList = metadataCollection.metadata; + if (metadataList.length == 0) { + throw new IllegalStateException("empty metadata: " + fileName); + } + if (metadataList.length > 1) { + logger.log(Level.WARNING, "invalid metadata (too many entries): " + fileName); } + PhoneMetadata metadata = metadataList[0]; + PhoneMetadata oldValue = map.putIfAbsent(key, metadata); + return (oldValue != null) ? oldValue : metadata; } /** - * Loads the metadata protocol buffer from the given stream and closes the stream afterwards. Any - * exceptions that occur while reading or closing the stream are ignored. - * - * @param source the non-null stream from which metadata is to be read. - * @return the loaded metadata protocol buffer. + * Loads and returns the metadata protocol buffer from the given stream and closes the stream. */ - private static PhoneMetadataCollection loadMetadataAndCloseInput(ObjectInputStream source) { + private static PhoneMetadataCollection loadMetadataAndCloseInput(InputStream source) { // The size of the byte buffer used for deserializing the phone number metadata files for each // region. final int MULTI_FILE_BUFFER_SIZE = 16 * 1024; - PhoneMetadataCollection metadataCollection = new PhoneMetadataCollection(); try { - metadataCollection.mergeFrom( - MetadataManager.convertStreamToByteBuffer(source, MULTI_FILE_BUFFER_SIZE)); + // Read in metadata for each region. + PhoneMetadataCollection metadataCollection = new PhoneMetadataCollection(); + metadataCollection.mergeFrom(MetadataManager.convertStreamToByteBuffer( + new ObjectInputStream(source), MULTI_FILE_BUFFER_SIZE)); + return metadataCollection; } catch (IOException e) { - logger.log(Level.WARNING, "error reading input (ignored)", e); + throw new RuntimeException("cannot load/parse metadata", e); } finally { try { source.close(); @@ -158,6 +152,5 @@ final class MultiFileMetadataSourceImpl implements MetadataSource { logger.log(Level.WARNING, "error closing input stream (ignored)", e); } } - return metadataCollection; } } diff --git a/java/libphonenumber/test/com/google/i18n/phonenumbers/MultiFileMetadataSourceImplTest.java b/java/libphonenumber/test/com/google/i18n/phonenumbers/MultiFileMetadataSourceImplTest.java index c1c531910..8bdd7913c 100644 --- a/java/libphonenumber/test/com/google/i18n/phonenumbers/MultiFileMetadataSourceImplTest.java +++ b/java/libphonenumber/test/com/google/i18n/phonenumbers/MultiFileMetadataSourceImplTest.java @@ -16,6 +16,9 @@ package com.google.i18n.phonenumbers; +import com.google.i18n.phonenumbers.nano.Phonemetadata.PhoneMetadata; + +import java.util.concurrent.ConcurrentHashMap; import junit.framework.TestCase; /** @@ -24,21 +27,38 @@ import junit.framework.TestCase; public class MultiFileMetadataSourceImplTest extends TestCase { public MultiFileMetadataSourceImplTest() {} + public void testGeographicalRegionMetadataLoadsCorrectly() { + ConcurrentHashMap map = new ConcurrentHashMap(); + PhoneMetadata metadata = MultiFileMetadataSourceImpl.loadMetadataFromFile( + "CA", map, "/com/google/i18n/phonenumbers/data/PhoneNumberMetadataProtoForTesting", + PhoneNumberUtil.DEFAULT_METADATA_LOADER); + assertEquals(metadata, map.get("CA")); + } + + public void testNonGeographicalRegionMetadataLoadsCorrectly() { + ConcurrentHashMap map = new ConcurrentHashMap(); + PhoneMetadata metadata = MultiFileMetadataSourceImpl.loadMetadataFromFile( + 800, map, "/com/google/i18n/phonenumbers/data/PhoneNumberMetadataProtoForTesting", + PhoneNumberUtil.DEFAULT_METADATA_LOADER); + assertEquals(metadata, map.get(800)); + } + public void testMissingMetadataFileThrowsRuntimeException() { - MultiFileMetadataSourceImpl multiFileMetadataSource = new MultiFileMetadataSourceImpl( - "no/such/file", PhoneNumberUtil.DEFAULT_METADATA_LOADER); // In normal usage we should never get a state where we are asking to load metadata that doesn't // exist. However if the library is packaged incorrectly in the jar, this could happen and the // best we can do is make sure the exception has the file name in it. try { - multiFileMetadataSource.loadMetadataFromFile("XX", -1); + MultiFileMetadataSourceImpl.loadMetadataFromFile( + "XX", new ConcurrentHashMap(), "no/such/file", + PhoneNumberUtil.DEFAULT_METADATA_LOADER); fail("expected exception"); } catch (RuntimeException e) { assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file_XX")); } try { - multiFileMetadataSource.loadMetadataFromFile( - PhoneNumberUtil.REGION_CODE_FOR_NON_GEO_ENTITY, 123); + MultiFileMetadataSourceImpl.loadMetadataFromFile( + 123, new ConcurrentHashMap(), "no/such/file", + PhoneNumberUtil.DEFAULT_METADATA_LOADER); fail("expected exception"); } catch (RuntimeException e) { assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file_123")); diff --git a/java/pending_code_changes.txt b/java/pending_code_changes.txt index 8b1378917..113cdddb0 100644 --- a/java/pending_code_changes.txt +++ b/java/pending_code_changes.txt @@ -1 +1,3 @@ - +Code changes: + - Simplify concurrent metadata loading in MultiFileMetadataSourceImpl and + reduce points of contention.