Skip to content

[Localization] Delay loading localization on startup #36705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 15, 2021

Conversation

HassanElDesouky
Copy link
Contributor

We were loading localization files on start-up, but now we need to stop loading them and instead load them when first diagnostic is requested, so there is no startup overhead for the compiler on projects without errors.

cc @xedin

@HassanElDesouky HassanElDesouky requested a review from xedin April 5, 2021 22:25
Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not exactly what we have discussed. I still don't think there is any need to save locale/path in the DiagnosticEngine, I think that belongs to LocalizationProducer as does most of the logic contained by DiagnosticEngine::setLocalization.

@HassanElDesouky HassanElDesouky force-pushed the delay-loading-localization branch 2 times, most recently from be18874 to 1dd38e9 Compare May 11, 2021 21:12
@HassanElDesouky HassanElDesouky requested a review from xedin May 12, 2021 20:12
Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better now, I have left some more comments inline.

- Add `LocalizationProducerState` to manage the states of LocalizationProducers.
- Add `initializeImpl` and `initializeIfNeeded` to manage lazily initialization of `LocalizationProducer`s.
- Move constructing a localization producer from DiagEngine to `LocalizationProrducer` itself.
@HassanElDesouky HassanElDesouky force-pushed the delay-loading-localization branch from c900057 to 481b29b Compare May 13, 2021 23:29
Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

@xedin
Copy link
Contributor

xedin commented May 14, 2021

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 481b29b

@xedin
Copy link
Contributor

xedin commented May 14, 2021

@swift-ci please clean test Linux platform

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 481b29b

@xedin
Copy link
Contributor

xedin commented May 15, 2021

@swift-ci please test Linux platform

@xedin xedin merged commit f69dfc2 into swiftlang:main May 15, 2021
@HassanElDesouky HassanElDesouky deleted the delay-loading-localization branch May 15, 2021 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants