Cache the era code maps #4199
Cache the era code maps #4199
Conversation
| .collect() | ||
| }); | ||
| map.get(calendar) | ||
| .unwrap_or_else(|| panic!("Unknown calendar {}", calendar)) |
There was a problem hiding this comment.
Nit: Keep the error message "Era map unknown for {calendar}" to make it more greppable
| ("0".to_string(), tinystr!(16, "roc-inverse")), | ||
| ("1".to_string(), tinystr!(16, "roc")), | ||
| fn get_era_code_map(calendar: &str) -> &'static BTreeMap<String, TinyStr16> { | ||
| static MAP: once_cell::sync::OnceCell<BTreeMap<String, BTreeMap<String, TinyStr16>>> = |
There was a problem hiding this comment.
We can't really do static caching, DatagenProviders can be mutated and are not singletons, this might bite us later. We have some caches on CldrCache, which itself is immutable.
There was a problem hiding this comment.
Yeah but this data is static. There's no way to affect this via changing CLDRCache
(The japanese era data used for symbols is static; it's verified by datagen but it's statically loaded)
There was a problem hiding this comment.
Ah right. Wouldn't it suffice to cache the result of crate::transform::cldr::calendar::japanese::compute_era_code_map? This is parsing the japanese file even if only gregorian is requested. Also, this doesn't need to return maps, the only call site just iterates over the pairs, so building BTreeMaps is overkill. For everything except japanese this can even be completely allocation-free if you pass a Fn((&str, TinyAsciiStr<16>)) closure, and then japanese can be cached only if it's requested.
There was a problem hiding this comment.
This is being reused in #4192 . We might be able to clean this up, I don't want to do that now; ideally we can do a second cleanup pass once the old symbols datagen code is removed.
There was a problem hiding this comment.
There's a lot of scope of improving this, I'm trying to incrementally improve its performance given that I'm writing code that will end up calling this map 6 times as often as we do today.
There was a problem hiding this comment.
Oh, I remember now, I didn't want to allocate the small BTreeMaps a ton of times.
We can switch to a different model without the BTreeMaps but I'd like to do that later
There was a problem hiding this comment.
I'll try a little bit
There was a problem hiding this comment.
Got it to return an iterator
No description provided.