Refactoring `Database`: `const` and faster loading, dropping bincode-loading

[bheylin]

At the moment the Database is quite a complex data structure.
I would like to make a single static database, but I'm not sure if that fit's with the design goals of the library.
So in an effort to clarify what the design goals of the library are, I have some questions:

• Is there a concrete use case for loading a database that's not the default?
  The fn parse_with accepts a database param, is that used by the community?

  • If there is no concrete case, then maybe the Database can become a static structure represented as Rust source code
    generated from the assets/PhoneNumberMetadata.xml?

• The bincode (Vec<Meatadata>) is processed on first use (through lazy_static at time of writing) into a Database which is a composite of a RegexCache and three HashMaps.
  This cache and HashMap generation has quite the runtime cost. It looks like it can be created offline and then stored as bincode. Then it's simply a matter of loading the database at runtime.

[rubdos]

Related to #26

[bheylin]

After the conversation in #62 I'll list some things to consider doing here:

• Deprecate any use of Databsae in the API
  Namely the three fns format_with, is_valid_with, parse_with

  This will allow us to change the Database structure without breaking the API.
  My concern here is that people are only using a custom Database because the default internal Database
  loads so slowly.

  The deprecation should prompt feedback from the projects/people using this lib.
  And give the maintainers time to discuss the reasons for deprecation.

• Remove the use of lazy_static / once_cell (Bump dependencies, follow rustfmt and clippy #60) and generate a const Database using the existing build.rs

  This will embed the Database into the lib binary transparently. Allowing for:

  • the removal of lazy_static dependency completely
  • moving both the bincode and quickxml from the [dependencies] section to build-dependencies
  • the transparent review of the contents of the Database.
  • the Database to load as fast as a lib binary can load (solving Loading default DATABASE is extremely slow in WASM #26)

[rubdos]

@bheylin In order to coordinate the effort a bit: I will make the separate issues asap (hopefully before noon CET), but I wonder which ones you intend to tackle yourself. Are you willing to take some of them on?

[bheylin]

Oh yea for sure I'll take some on 😆

Otherwise I'd just be complaining and delegating work, and I'm just not ready for a toxic management role yet 😆

I would like to keep the definition of the Database the same for now and use mod loader and Database::from in the build.rs and see where that leads.

I think that will allow us to avoid making breaking changes to the API while moving the Databsae a step closer to being static.

So in short, give me this task and I'll make several PR's over time that take steps towards a static DB.
We can then decide what point we change the public API.

[rubdos]

That sounds like a good path to follow. Keep the PR's small and concise, commit often, and ask feedback early! That way we'll reach a consensus with the least friction possible.

[bheylin]

@rubdos I'm taking a look at how we could represent the database and it would help me out if I knew what the minimal API phonenumber needs to support.

From what I see the critical/domain API is the set of functions format, is_valid, is_viable and parse.
And associated types PhoneNumber, Carrier, Extension, Formatter, NationalNumber. Mode, Type and Validation.

I mentioned before that I would like to make Database private and most likely restructure it so it can load faster.
My question now is:

Does the metadata module need to be public?
I don't see a use case for the consumer of the library needing it.

As by making the Databsae and metadata types private, it gives us the freedom to restructure the Database / metadata's layout completely.

[bheylin]

@rubdos If we use build.rs to generate a databsae.rs file the existing Database::regions HashMap can be converted into a match based fn.

fn regions_by_code(code: u16) -> Option<&'static [&'static str]> {
    match code {
        1 => Some(&[
            "US", "AG", "AI", "AS", "BB", "BM", "BS", "CA", "DM", "DO", "GD", "GU", "JM", "KN",
            "KY", "LC", "MP", "MS", "PR", "SX", "TC", "TT", "VC", "VG", "VI",
        ]),
        7 => Some(&["RU", "KZ"]),
        // ... and many more variants
        998 => Some(&["UZ"]),
        _ => None,
    }
} 

The Database::by_code HashMap can be replaced by a match based fn.

fn id_by_code(country_code: u16) -> Option<&'static str> {
    match country_id {
        // ...
        247 => Some("AC"),
        // ...
        979 => Some("001"),
        // ....
        _ => None
    }
}

The country_id can then be given to another match based fn that has Metadata hardcoded into the match statement.

fn metadata_by_id(country_id: &str) -> Option<Metadata> {
    match country_code {
        "001" => Some(Metadata { ... })
        // ... This match statement will be a big fella
        _ => None
    }
}

The representation of the Metadata will need to change ofc as we want all static data in that too.
That's something I need to play around with.

#[derive(Clone, Debug)]
pub struct Metadata {
    pub(crate) descriptors: Descriptors,
    pub(crate) id: &'static str,
    pub(crate) country_code: u16,

    pub(crate) international_prefix: Option<CachedRegex>,
    pub(crate) preferred_international_prefix: Option<&'static str>,
    pub(crate) national_prefix: Option<&'static str>,
    pub(crate) preferred_extension_prefix: Option<&'static str>,
    pub(crate) national_prefix_for_parsing: Option<CachedRegex>,
    pub(crate) national_prefix_transform_rule: Option<&'static str>,

    pub(crate) formats: &'static [Format],
    pub(crate) international_formats: &'static [Format],
    pub(crate) main_country_for_code: bool,
    pub(crate) leading_digits: Option<CachedRegex>,
    pub(crate) mobile_number_portable: bool,
}

I can build a parallel version of this database representation and compare the performance to HashMap look ups.
My hope is that the match statements are converted to jump tables.

I have to think about how to handle the CachedRegex in general.

[rubdos]

At first, I would avoid breaking the API. Maybe we can decorate some things with #[deprecated] at some point, but even that I would do in a follow-up version I'd say.

In your proposal, I would be 99% sure that those would become jump tables. Then again, as far as I understood, the performance problem is in loading the Database, not in looking things up? If so, I would do a first iteration where you keep the original format (again, backwards compatibility, although those are private types?), and create some criterion benchmarks around that format.

If the representation of Metadata needs to change, let's definitely not enforce 'static, because then dynamic database will not be possible and this will break API.

The regex can probably also be done on compile time; the regex crate is quite good :-)

[bheylin]

Yea I've made a branch locally to test out the ramifications of replacing the HashMaps and as expected it involves a lot of churn.

The point of the match / 'static data as HashMap replacements is not only about lookup time, as the creation of the various HashMaps involve a fair amount of heap allocations. I'm presuming that this is slowing down the init phase, but I haven't measured that yet.

I thinking about making a trimmed down Database like type that has no RegexCache as RegexCache cannot be serialized. This partial database can then be serialized to the database.bin. Now the database.bin contains a Vec<Metadata> which is processed into a Database at first load. I'm presuming that all the HashMap creation is causing the slowdown.

What do you think about that as a first step?