Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Improve Translator caching#28937

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromrpkamp:translator-cache
Apr 6, 2019
Merged

Conversation

@rpkamp
Copy link
Contributor

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#27600
LicenseMIT
Doc PRN/A

Add DirectoryResources to MessageCatalogues when loaded.

So that when a file is added to one of the directories the cache for all MessageCatalogues will be invalidated.

All directories must be added to all MessageCatalogues because we can't predict for which locale files will be added to each individual directory.

Also, now that the translator keeps track of its own directories for caching the container no longer needs to it. This means that when a translation changes or is added the container no longer needs to be fully rebuilt, saving a considerable amount of time (compilation time went down from ~4 seconds to ~1 second on each translation change/add in our project).

welcoMattic and vudaltsov reacted with thumbs up emoji
@rpkamp
Copy link
ContributorAuthor

Rebased and force pushed to fix merge conflict.

Could I get a review here please? 🙂

@rpkamp
Copy link
ContributorAuthor

Rebased again.

Could you have a look please@nicolas-grekas? 🙂

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Looks good thanks. Here are some comments. Would it be possible to add some test cases also?

@rpkamprpkampforce-pushed thetranslator-cache branch 3 times, most recently from039ecd6 tod7ed632CompareMarch 22, 2019 20:47
@rpkamp
Copy link
ContributorAuthor

@nicolas-grekas I've rebased onto master and stopped usingContainerBuilder::fileExists() for translation resources altogether since the container no longer needs to track any changes in translation files - catalogues are fully handling that now.

This prevents a full container rebuild if any of the translation directories it was tracking but did not exist is created - it will only rebuild the Catalogue in that case, which is much faster.

I've also added some tests.

Could you check again please? 🙂

welcoMattic reacted with thumbs up emoji

@rpkamp
Copy link
ContributorAuthor

Failing tests are unrelated (I think).

@welcoMattic
Copy link
Member

Hi@rpkamp, I have a similar issue that your PR could solve.Here the use case and the steps to reproduce it. I'll try your PR with my scenario to test if it solves the problem.

Do you have a scenario to be reproduced?

Thanks

@rpkamp
Copy link
ContributorAuthor

This PR makes sure that catalogues also track directories that were found empty when scanning for translation files, where they didn't before.

So yes, if I understand correctly this PR is also a fix for your problem,@welcoMattic 🙂

welcoMattic reacted with thumbs up emoji

@rpkamp
Copy link
ContributorAuthor

Rebased on master again. This time the failed build on Travis is definitely unrelated 🙂

Ping@nicolas-grekas, could you review again please?

@stof
Copy link
Member

stof commentedApr 1, 2019

The list of files being loaded is still built by the container. So AFAICT, this change does not allow removing the resources at the container-level (we would still need to build the new list).
The only benefit of that change would be that the translator cache would be invalidated when adding new files too, solving#27600 (which is already a good move).
Some attempt was done in the past to include a hash of the list of resource files in the cache key, but this was reverted due to bad effects.

@rpkamp
Copy link
ContributorAuthor

So AFAICT, this change does not allow removing the resources at the container-level (we would still need to build the new list).

This change is already removing the resources from the container, and everything works fine without the container knowing where translations files reside. Why would it need to know about those?

The only reason I can think of is if I add another translations path to the framework, but then the resource where I define that is not fresh anymore, so the container would be rebuilt, and the change would cascade to the catalogues.

@rpkamp
Copy link
ContributorAuthor

Comments processed@fabpot. Could you check again please?

@curry684
Copy link
Contributor

Think it's right now but@fabpot should have a quick look still.

@fabpot
Copy link
Member

Thank you@rpkamp.

rpkamp reacted with heart emoji

@fabpotfabpot merged commita524658 intosymfony:masterApr 6, 2019
fabpot added a commit that referenced this pull requestApr 6, 2019
This PR was squashed before being merged into the 4.3-dev branch (closes#28937).Discussion----------Improve Translator caching| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#27600| License       | MIT| Doc PR        | N/AAdd DirectoryResources to MessageCatalogues when loaded.So that when a file is added to one of the directories the cache for all MessageCatalogues will be invalidated.All directories must be added to all MessageCatalogues because we can't predict for which locale files will be added to each individual directory.Also, now that the translator keeps track of its own directories for caching the container no longer needs to it. This means that when a translation changes or is added the container no longer needs to be fully rebuilt, saving a considerable amount of time (compilation time went down from ~4 seconds to ~1 second on each translation change/add in our project).Commits-------a524658 Improve Translator caching
@curry684
Copy link
Contributor

@fabpot label? ;)

@fabpot
Copy link
Member

@curry684 oops, fixed

@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
OskarStark added a commit to symfony/symfony-docs that referenced this pull requestJul 29, 2019
…ache for translation files (javiereguiluz)This PR was merged into the 4.3 branch.Discussion----------[Translation] Mention that you don't have to clear the cache for translation filesMentions the nice new behavior implemented insymfony/symfony#28937.Commits-------ddf97c7 [Translation] Mention that you don't have to clear the cache for translation files
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@sstoksstoksstok left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

9 participants

@rpkamp@welcoMattic@stof@curry684@fabpot@nicolas-grekas@sstok@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp