Adding LruDataCache and overlay examples #2914

sffc · 2022-12-23T01:20:13Z

I don't expect a review on any PRs I send this week until January

robertbastian

I don't think this is a good example. It ignores all the zero-copying that we've put a lot of effort into, and even needs to clone the locale to do lookup. Let's restrict the example to wrapping an AnyProvider, and put more work into AnyProvider <-> BufferProvider interaction so that we don't allocate left and right.

docs/tutorials/data_provider.md

sffc · 2022-12-23T13:45:09Z

I don't think this is a good example. It ignores all the zero-copying that we've put a lot of effort into, and even needs to clone the locale to do lookup. Let's restrict the example to wrapping an AnyProvider, and put more work into AnyProvider <-> BufferProvider interaction so that we don't allocate left and right.

I disagree on the following points:

It definitely does not ignore zero-copy effort; the yoke is intact when stored and retrieved from the cache. Cache retrievals would not be cheap without the help of Yoke and ZeroVec.
The DataLocale clone aside, there is a single additional allocation during storage (DataResponse to AnyResponse) and none during retrieval thanks to the Rc Cart. It very much mischaracterizes this example to say that we "allocate left and right".
I updated the example to avoid cloning the DataLocale during retrieval; it adds a bit of complexity for readers, which I was trying to avoid.
This is I think one of the best examples of how BufferProvider and AnyProvider can interoperate, and I chose a BufferProvider source specifically to make that point. As we've discussed before, there is nothing we can do to make BufferProvider and AnyProvider interoperate any better than this short of going through the registry. A cache that implements DataProvider can invoke both BufferProvider and AnyProvider, which is something worth demonstrating.

robertbastian · 2022-12-23T14:34:22Z

docs/tutorials/data_provider.md

+ICU4X has no internal caches because there is no one-size-fits-all solution. It is easy for clients to implement their own cache for ICU4X, and although this is not generally required or recommended, it may be beneficial when latency is of utmost importance and:
+
+1. A less-efficient data provider is being used, such as JSON
+2. Lookups make heavy use of vertical fallback


thought: I don't think we should suggest caching the data structs when fallback is the problem. In that case one should cache resolved locales

The resolved locales could be different for each key. But yes, we could cache (DataKey, DataLocale) -> DataLocale.

I removed the use case of vertical fallback since this is a good point and we could even consider offering this as an official solution if needed.

robertbastian · 2022-12-23T14:37:34Z

docs/tutorials/data_provider.md

@@ -121,6 +145,7 @@ assert_eq!(provider.cache.lock().unwrap().len(), 0);

 assert_eq!(
    "こんにちは世界",
+    // Note: It is necessary to use `try_new_unstable` with LruDataCache.


thought: this seems weird from client perspective. If we provided a cache we could actually make it implement AnyProvider

I would love to hear how you think we could implement a cache implementing AnyProvider on top of an inner provider that implements BufferProvider.

With the registry we can do it, no?

Exactly; using the registry is the only way one could do it. Maybe we don't align on whether the registry is a good solution; I think it's a solution that is generally quite bad and should be avoided if there is another viable option. In this case, we can put the cache into userland to circumvent the need for the registry.

docs/tutorials/data_provider.md

This reverts commit bfe97f4.

sffc · 2022-12-23T15:44:35Z

Changed this PR to "Fixes #1246"

robertbastian · 2022-12-23T19:25:51Z

docs/tutorials/data_provider.md

+            }
+        }
+        // Release the lock to invoke the inner provider
+        let response: DataResponse<M> = self.provider.load(req)?;


Is this type needed? self.provider is only DataProvider<M>, so there shouldn't be any ambiguity.

Primarily for clarity. The audience for this tutorial doesn't know what all these functions do or what they return. But if you think it's clear enough I'll remove it.

robertbastian · 2022-12-23T19:28:54Z

docs/tutorials/data_provider.md

+            let mut res: DataResponse<DecimalSymbolsV1Marker> = any_res.downcast()?;
+            if let Some(payload) = &mut res.payload.as_mut() {
+                payload.with_mut(|data| {
+                    // Change the decimal separators for all Swiss locales to '.' and ','


You seem to be implying that our data needs to be fixed. Maybe replace them by something fun, like 🐮

Mooooo 🐮

Manishearth · 2022-12-24T22:27:48Z

docs/tutorials/data_provider.md

@@ -61,3 +61,169 @@ impl AdditiveIdentity {
    }
 }
 ```
+
+## Caching Data Provider


suggestion: kinda want this to be a separate page because it's so complicated

though perhaps that can be kept in mind for mdbookifying?

Filed #2929 for mdbook, and we can track this there

I wasn't exactly sure the best place to put this: it's a bit low-level/detailed for a tutorial, but it's also too big/off-topic for a docs page.

yeah i think in the mdbook world we can make this a subchapter

Manishearth · 2022-12-28T19:06:02Z

docs/tutorials/data_provider.md

+struct CacheKeyWrap(CacheKey<'static>);
+
+#[derive(Debug, PartialEq, Eq, Hash)]
+struct CacheKey<'a>(DataKey, Cow<'a, DataLocale>);


suggestion: comment explanation on this section about how the borrow works here to make it possible to do non-cloning gets (probably pair CacheKey with the Borrow impl in code organization too)

Yeah. I'm not proud of this code because it uses a #[doc(hidden)] export, lru::KeyRef. It's confusing and distracting to have it in the example. I originally skipped this and just cloned the key for lookup, but @robertbastian's review feedback was that my code was cloning too much. I can labor over explaining how the key borrowing works, but it's not the point of this example.

Yeah I think we shouldn't explain it in depth but every line of code in an example is something the reader may focus on: I think we should move the Borrow impl and the borrowed key type below the main stuff with a one-liner comment that just says it's to avoid clones on lookup , so that the reader doesn't spend too much time trying to figure out how it fits in to the rest

robertbastian · 2023-01-02T17:06:53Z

docs/tutorials/data_provider.md

+        let mut any_res = self.0.load_any(key, req)?;
+        if key == DecimalSymbolsV1Marker::KEY && req.locale.region() == Some(region!("CH")) {
+            let mut res: DataResponse<DecimalSymbolsV1Marker> = any_res.downcast()?;
+            if let Some(payload) = &mut res.payload.as_mut() {


I was wondering what the error case here is, but there doesn't seem to be one. The design decision to make payload an Option<DataPayload> doesn't seem to have paid off, it's never None. Maybe unwrap here so readers don't have to figure this out themselves.

We should consider making the payload non-optional for 2.0 if there's still no use case for an absent payload by then.

Adding LruDataCache example

48b9022

sffc requested review from Manishearth and a team as code owners December 23, 2022 01:20

sffc requested a review from robertbastian December 23, 2022 01:20

robertbastian requested changes Dec 23, 2022

View reviewed changes

docs/tutorials/data_provider.md Outdated Show resolved Hide resolved

docs/tutorials/data_provider.md Outdated Show resolved Hide resolved

docs/tutorials/data_provider.md Outdated Show resolved Hide resolved

docs/tutorials/data_provider.md Outdated Show resolved Hide resolved

sffc added 4 commits December 23, 2022 05:06

Add more explanatory text

88470e7

fmt

dc397af

Avoid cloning the DataLocale on lookup

d633a2c

Refactoring

a24b19d

sffc requested a review from robertbastian December 23, 2022 13:45

sffc added 3 commits December 23, 2022 05:52

req/res

5f2f069

comp_res and one-liner cache storage

f6a0e10

downcast_cloned for consistency with map_project_cloned

398bdce

robertbastian reviewed Dec 23, 2022

View reviewed changes

docs/tutorials/data_provider.md Show resolved Hide resolved

sffc added 7 commits December 23, 2022 07:00

DisplayNames fixes

bfe97f4

DisplayNames overwrite example

47212b7

Use a stable component (icu_decimal) for the example

3358185

Remove line item about vertical fallback

3d3b6e2

Revert "DisplayNames fixes"

0481da9

This reverts commit bfe97f4.

Back to computed_res

9cd296b

ok, response

6100e72

sffc requested a review from robertbastian December 23, 2022 15:39

fix comment

2ab3d68

sffc changed the title ~~Adding LruDataCache example~~ Adding LruDataCache and overlay examples Dec 23, 2022

robertbastian reviewed Dec 23, 2022

View reviewed changes

Review feedback

4332ade

sffc requested a review from robertbastian December 23, 2022 22:15

Manishearth mentioned this pull request Dec 24, 2022

Move tutorials and docs to mdbook #2929

Open

Manishearth reviewed Dec 24, 2022

View reviewed changes

Manishearth approved these changes Dec 28, 2022

View reviewed changes

robertbastian approved these changes Jan 2, 2023

View reviewed changes

sffc merged commit da0b424 into unicode-org:main Jan 5, 2023

sffc deleted the docs-1.1 branch January 5, 2023 20:57

sffc mentioned this pull request Jan 5, 2023

Improve comments on borrow types in LRU example #2959

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding LruDataCache and overlay examples #2914

Adding LruDataCache and overlay examples #2914

sffc commented Dec 23, 2022 •

edited

Loading

robertbastian left a comment

sffc commented Dec 23, 2022

robertbastian Dec 23, 2022

sffc Dec 23, 2022

sffc Dec 23, 2022

robertbastian Dec 23, 2022 •

edited

Loading

sffc Dec 23, 2022

robertbastian Dec 23, 2022

sffc Dec 23, 2022

sffc commented Dec 23, 2022

robertbastian Dec 23, 2022

sffc Dec 23, 2022

robertbastian Dec 23, 2022

sffc Dec 23, 2022

Manishearth Dec 24, 2022

Manishearth Dec 24, 2022

sffc Dec 26, 2022

Manishearth Dec 26, 2022

Manishearth Dec 28, 2022

sffc Dec 29, 2022

Manishearth Dec 29, 2022

robertbastian Jan 2, 2023

sffc Jan 5, 2023

Adding LruDataCache and overlay examples #2914

Adding LruDataCache and overlay examples #2914

Conversation

sffc commented Dec 23, 2022 • edited Loading

robertbastian left a comment

Choose a reason for hiding this comment

sffc commented Dec 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Dec 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Dec 23, 2022 •

edited

Loading

robertbastian Dec 23, 2022 •

edited

Loading