Should we provide a caching solution for DataProvider?

[nordzilla]

Relevant Background/Related Discussions:

• Ergonomic API & Data Providers #30

Discussion

In the experimentation with ICU4X in Gecko and SpiderMonkey, we saw that the ICU4X PluralRules constructor (with no number formatter) was slower than the ICU4C PluralRules constructor (with no number formatter):

In our Firefox code, we created a singleton DataProvider instance:

/* static */
Provider* Provider::GetInstance() {
  if (mProviderSingleton == nullptr) {
    mProviderSingleton = new Provider();
    // TODO! Hard coded path!
    const char* path = "/tmp/icu4x_data/bincode";
    ICU4XCreateDataProviderResult result =
        icu4x_fs_data_provider_create(path, strlen(path));
    MOZ_RELEASE_ASSERT(result.success);
    mProviderSingleton->mProvider = result.provider;
  }
  return mProviderSingleton;
}

ICU4XDataProvider* Provider::AsFFI() { return &mProvider; }

We found in the profiler that even with a singleton on the client side, the instance of ICU4X's data provider still reads the bincode data from the file system for every constructed PluralRules object.

Currently, in ICU4X, resolving the PluralRules data calls load_payload() every time:

pub fn resolve_plural_data<'d, 's, D: DataProvider<'d, 's, PluralRuleStringsV1Marker> + ?Sized>(
    langid: LanguageIdentifier,
    data_provider: &D,
    type_: PluralRuleType,
) -> Result<DataPayload<'d, 's, PluralRuleStringsV1Marker>, PluralRulesError> {
    let key = match type_ {
        PluralRuleType::Cardinal => super::key::CARDINAL_V1,
        PluralRuleType::Ordinal => super::key::ORDINAL_V1,
    };
    Ok(data_provider
        .load_payload(&DataRequest {
            resource_path: ResourcePath {
                key,
                options: ResourceOptions {
                    variant: None,
                    langid: Some(langid),
                },
            },
        })?
        .take_payload()?)
}

In the ICU4C profile for PluralRules::TryCreate() we collected 415 samples.
In the ICU4X profile for PluralRules::TryCreate() we collect 872 samples.

If we ignore the samples that read from the file system in the ICU4X profile it drops the total sample count for PluralRules::TryCreate() to 135.

The takeaway here is that the ICU4X PluralRules constructor could be faster than the ICU4C constructor with some implementation of caching the data.

But would this be worth the extra memory overhead?

Even with caching, the data has to be read from the file system at least once, so if common use cases only create a single PluralRules object, caching is probably not worth the effort.

An option to consider might be to add caching as an opt-in compile-time feature for clients who know that they will be constructing many objects from the same data. I imagine that this would affect every object that requires DataProvider, and not just PluralRules.

[sffc]

Short answer: this is a known issue with PluralRules (#854), not an issue with caches or data provider.

Long answer: ICU4C's caching is a complete mess. We absolutely do not want to replicate it in ICU4X. Instead of caching full objects, I have been advocating for making constructors as cheap as possible, and allowing clients to configure caching, if desired, in the data pipeline. I discuss more about the philosophy around caching here, in data_pipeline.md.

But would this be worth the extra memory overhead?

Maybe not in Android or WearOS. Even on servers, the costs of RAM are rising relative to the costs of CPU. I strongly believe it needs to be a business decision by each individual client. One of ICU4C's big mistakes is assuming that there is a one-size-fits-all approach to the space-time tradeoff.

[sffc]

Also:

Even with caching, the data has to be read from the file system at least once

Once the in-memory data provider is ready (static done in #776, non-static tracked by #848), we don't need to touch the filesystem, which should improve performance as well.