Use icu shards instead of loading icudt.dat #25521
Conversation
ghost
added
the
| }, | ||
|
|
||
| beginHeapLock: function() { | ||
| beginHeapLock: function () { |
There was a problem hiding this comment.
Sorry about this - I formatted the file and it made a bunch of changes. Use the Hide whitespaces option to avoid seeing unnecessary changes.
There was a problem hiding this comment.
No problem. Best not to fight the tools :) If this is how VS code wants to format it, let's do it like that!
| // culture in client-side Blazor. | ||
| // This type is internal since localization currently does not work. | ||
| // Make it public onc https://github.com/dotnet/runtime/issues/38124 is resolved. | ||
| internal class WebAssemblyGlobalizationTest : GlobalizationTest<ToggleExecutionModeServerFixture<Program>> |
pranavkm
force-pushed
the
prkrishn/sharding
branch
from
67954bd to
5cb678d
Compare
pranavkm
requested review from
Tratcher,
dougbu,
halter73 and
jkotalik
as code owners
pranavkm
force-pushed
the
prkrishn/sharding
branch
from
5cb678d to
2e3a7ba
Compare
| { | ||
| // Arrange | ||
| // This verifies the non-CJK icu data set. | ||
| var culture = new CultureInfo("kn"); |
There was a problem hiding this comment.
@lewing \ @EgorBo I authored a few tests to verify that Blazor is loading icu resources correctly one for each of the 3 shards. I verified that we're loading the right shard (and there's another test that verifies this for the ru culture), but this one appears to be failing.
Assert.Equal() Failure
↓ (pos 1)
Expected: 2/9/2020 12:00:00 ಪೂರ್ವಾಹ್ನ
Actual: 2020-09-02 00:00:00
There was a problem hiding this comment.
@pranavkm to be clear: is kn not expected to work in non-CJK icudt?
There was a problem hiding this comment.
@EgorBo I would have expected it to work. This doesn't seem specific to the shards. Even if I load icudt.dat, there's a bunch of different cultures for which it looks like culture data is missing.
There was a problem hiding this comment.
@pranavkm sorry for the delay, the fix is here dotnet/icu#34
I guess we need to backport it to 5.0rc2 ?
There was a problem hiding this comment.
the bug affects the following locales: "bn", "et", "gu", "kn", "ml", "mr", "ms", "no", "ta", "te"
There was a problem hiding this comment.
I guess we need to backport it to 5.0rc2 ?
We could definitely try asking. The risk is fairly limited and we have the opportunity to get better user feedback..
SteveSandersonMS
left a comment
SteveSandersonMS
left a comment
There was a problem hiding this comment.
Looks good!
Really great job on the E2E tests here. That must have been quite difficult.
pranavkm
force-pushed
the
prkrishn/sharding
branch
from
2e3a7ba to
f4b24f2
Compare
|
@Pilchie, could you approve this for rc2? |
pranavkm
force-pushed
the
prkrishn/sharding
branch
from
62b67bc to
8c20e40
Compare
|
Approved for .NET 5. RC2. Are there any customers we can get to try out nightly builds once this is an installer to get some early validation and help us feel more comfortable with these changes. |
|
@Pilchie we're going to reach out some community members on out Gitter and ask them to try out nightlies once they become available. |
pranavkm
force-pushed
the
prkrishn/sharding
branch
from
8c20e40 to
5fd00e0
Compare
Description
.NET runtime started shipping globalization data in shards. Loading the shard based on the browser culture reduces the WASM download size which is desirable.
Customer impact
Reduced download size
Risk
Medium. Apps that configure the culture in .NET would require opting out of this feature. We attempt to detect this case, and we have tests for it, but we haven't seen how it behaves in the wild.
This change defaults Blazor to load icu shards rather than the full icu data dropping the size to 2046.15 KB from the current 2220.16 KB (~160KB reduction in size).
One of the issues that is being investigated is that loading icu data needs to happen using ambient data before the runtime has been loaded and not the .NET specified culture. We use navigator.languages which is the same property that is used by Emscripten to set the default culture. In addition, I've added a JS API that will allow users to specify a culture as part of initializing Blazor WebAssembly. This may help in the niche cases where the culture is readily known but users do not want to pay the cost of loading the full icu data.
I did not want to rely on the new API for our tests until we were happy with it. At the same time configuring browsers to launch with a specific culture requires all other browser instances to be gone. So writing a test that exercises the non-EFIGS data is a bit challenging.