Improving icu_harfbuzz compiled data

[robertbastian]

The current HB compiled data code is not ideal, because it uses static_to_owned, which might be blocking optimisations. This also let me realise that some compiled constructors that could be const aren't.

[sffc]

Before I review this, were you aware of #3257, servo/rust-harfbuzz#197, and servo/rust-harfbuzz#245 when writing this PR?

[robertbastian]

I found it after. I do think the changes here are independent of that though, we'll need a compiled data and a data provider path if we want maximum performance.

[sffc]

Not convinced this PR is an improvement.

1. It creates two versions of the icu4x_hb_unicode_* functions, which seem to be one for static data and one for provider data. We try really hard to avoid cfg(not(...)) code because it's very difficult to ensure test coverage.
2. The PR seems to be based on the claim that the current code "uses static_to_owned, which might be blocking optimisations". I'm fairly concerned about this statement because static_to_owned is clearly documented as being "cheap", and it is a const function. If there is a problem with that function, should we go back to the drawing board? It is designed, I believe, specifically for cases like this where we want to coalesce static and dynamic data down to a single code path.

[robertbastian]

The PR seems to be based on the claim that the current code "uses static_to_owned, which might be blocking optimisations". I'm fairly concerned about this statement because static_to_owned is clearly documented as being "cheap", and it is a const function. If there is a problem with that function, should we go back to the drawing board? It is designed, I believe, specifically for cases like this where we want to coalesce static and dynamic data down to a single code path.

Coalescing static data into the dynamic codepath inherently blocks optimisations. As soon as a DataPayload gets involved, all the non-static code gets pulled in, and the enum switch inhibits any possible optimisations against the static data. static_to_owned itself is cheap, because creating a static DataPayload is cheap, but we never claimed that it's as efficient as using the fully static codepath.

[sffc]

I still don't really like the cfg(not(...)) code. Can we instead export two different C functions with different names, one which reads from user_data and one which reads from compiled data? And then export two versions of Rust pub fn new_hb_unicode_funcs, one which uses compiled data and the other which uses dynamic data. The compiled data versions are behind the compiled_data feature; it's just that the dynamic data versions are still available even if the compiled_data feature is enabled.

[sffc]

The code duplication is annoying but hopefully that will go away with #3257

However, note that servo/rust-harfbuzz#197 always uses user_data. It takes the void* and casts it to the generic type parameter and calls the trait function with that pointer as the self parameter. So, migrating this code over to the new harfbuzz traits will be slightly different. I guess we would just ignore the self parameter in the compiled_data code path.

I assume you did not touch any of the unsafe FFI logic.

[sffc]

This works, with the Rust fns all named cb? I guess the names get mangled? Nice

[robertbastian]

Yep