Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 6, 2019
Merged

Improve Translator caching #28937

merged 1 commit into from
Apr 6, 2019

Conversation

rpkamp
Copy link
Contributor

@rpkamp rpkamp commented Oct 20, 2018

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/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).

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 20, 2018
@rpkamp rpkamp mentioned this pull request Oct 20, 2018
Closed
@rpkamp
Copy link
Contributor Author

rpkamp commented Dec 16, 2018

Rebased and force pushed to fix merge conflict.

Could I get a review here please? 🙂

@rpkamp
Copy link
Contributor Author

rpkamp commented Jan 22, 2019

Rebased again.

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

nicolas-grekas
nicolas-grekas reviewed Jan 25, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-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?

@rpkamp rpkamp force-pushed the translator-cache branch 3 times, most recently from 039ecd6 to d7ed632 Compare March 22, 2019 20:47
@rpkamp
Copy link
Contributor Author

rpkamp commented Mar 22, 2019

@nicolas-grekas I've rebased onto master and stopped using ContainerBuilder::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? 🙂

@rpkamp rpkamp force-pushed the translator-cache branch from 9c9b618 to bf6afed Compare March 24, 2019 19:08
@rpkamp
Copy link
Contributor Author

rpkamp commented Mar 26, 2019

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
Contributor Author

rpkamp commented Mar 29, 2019

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 🙂

@rpkamp rpkamp force-pushed the translator-cache branch from bf6afed to c4b40e3 Compare March 30, 2019 08:26
@rpkamp
Copy link
Contributor Author

rpkamp commented Mar 30, 2019

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 commented Apr 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
Contributor Author

rpkamp commented Apr 1, 2019

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
Contributor Author

rpkamp commented Apr 3, 2019

Comments processed @fabpot. Could you check again please?

@curry684
Copy link
Contributor

curry684 commented Apr 6, 2019

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

@fabpot fabpot force-pushed the translator-cache branch from ff793fe to a524658 Compare April 6, 2019 18:37
@fabpot
Copy link
Member

fabpot commented Apr 6, 2019

Thank you @rpkamp.

@fabpot fabpot merged commit a524658 into symfony:master Apr 6, 2019
fabpot added a commit that referenced this pull request Apr 6, 2019
@fabpot
feature #28937 Improve Translator caching (rpkamp)
a68b4c7 
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/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).

Commits
-------

a524658 Improve Translator caching
@curry684
Copy link
Contributor

curry684 commented Apr 6, 2019

@fabpot label? ;)

@fabpot
Copy link
Member

fabpot commented Apr 6, 2019

@curry684 oops, fixed

@deguif deguif mentioned this pull request Apr 7, 2019
Closed
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
Merged
@javiereguiluz javiereguiluz mentioned this pull request Jul 24, 2019
Closed
@nicolas-grekas nicolas-grekas mentioned this pull request Jul 24, 2019
Merged
@javiereguiluz javiereguiluz mentioned this pull request Jul 26, 2019
Merged
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Jul 29, 2019
@OskarStark
minor #12021 [Translation] Mention that you don't have to clear the c…
7efb02d 
…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 files

Mentions the nice new behavior implemented in symfony/symfony#28937.

Commits
-------

ddf97c7 [Translation] Mention that you don't have to clear the cache for translation files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@sstok sstok sstok left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Feature Status: Reviewed Translation
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