Skip to content
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

Improve Translator caching #28937

Merged
merged 1 commit into from Apr 6, 2019

Conversation

Projects
None yet
9 participants
@rpkamp
Copy link
Contributor

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

@rpkamp rpkamp force-pushed the rpkamp:translator-cache branch from 702a748 to 5a4d9dc Oct 20, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 20, 2018

@rpkamp rpkamp force-pushed the rpkamp:translator-cache branch from 5a4d9dc to 1499edc Oct 21, 2018

@rpkamp rpkamp force-pushed the rpkamp:translator-cache branch from 1499edc to 28da4d2 Dec 16, 2018

@rpkamp

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2018

Rebased and force pushed to fix merge conflict.

Could I get a review here please? 🙂

@rpkamp rpkamp force-pushed the rpkamp:translator-cache branch from 28da4d2 to 6fdd6d3 Jan 22, 2019

@rpkamp

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

Rebased again.

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

@nicolas-grekas
Copy link
Member

left a comment

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

@rpkamp rpkamp force-pushed the rpkamp:translator-cache branch 3 times, most recently from 039ecd6 to d7ed632 Mar 22, 2019

@rpkamp

This comment has been minimized.

Copy link
Contributor Author

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 rpkamp:translator-cache branch from 9c9b618 to bf6afed Mar 24, 2019

@rpkamp

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Failing tests are unrelated (I think).

@welcoMattic

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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 rpkamp:translator-cache branch from bf6afed to c4b40e3 Mar 30, 2019

@rpkamp

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

Comments processed @fabpot. Could you check again please?

@curry684

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

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

@fabpot

fabpot approved these changes Apr 6, 2019

@fabpot fabpot force-pushed the rpkamp:translator-cache branch from ff793fe to a524658 Apr 6, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Thank you @rpkamp.

@fabpot fabpot merged commit a524658 into symfony:master Apr 6, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 6, 2019

feature #28937 Improve Translator caching (rpkamp)
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

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

@fabpot label? ;)

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

@curry684 oops, fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.