[2.7][Translation] added message cache. #13986

aitboudad · 2015-03-19T19:59:00Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Fixed tickets	#13948, #2570, #13676
Tests pass?	yes
License	MIT

tests
add MessageDoctrineCache

# app/config/config.yml
framework:
    translator:
        cache: translation.cache.doctrine # translation.cache.default

xabbuh · 2015-03-20T00:01:24Z

src/Symfony/Component/Translation/Translator.php

@@ -76,11 +70,15 @@ class Translator implements TranslatorInterface, TranslatorBagInterface
     *
     * @api
     */
-    public function __construct($locale, MessageSelector $selector = null, $cacheDir = null, $debug = false)
+    public function __construct($locale, MessageSelector $selector = null, $cache = null, $debug = false)


The docblock needs to be changed to take into account that $cache can be an instance of MessageCacheInterface.

aitboudad · 2015-03-26T14:26:50Z

I think this could be a nice addition for 2.7, the default cache is useful in 80% of apps but when the size of catalogue became more than 10 MB it's consumes too much memory.
So the solution is to use DoctrineMessageCache, I tested with a catalogue(~100 mb) and the result look really interesting:

#config.yml
framework:
    translator:      { fallbacks: [en], cache: translation.cache.doctrine }

doctrine_cache:
    providers:
        translation_cache:
            memcached:
                servers:
                    localhost: 11211

#services.yml
translation.cache.doctrine:
    class: Symfony\Component\Translation\DoctrineMessageCache
    arguments: [%kernel.debug%, @translation_cache]

https://blackfire.io/profiles/compare/82c496fe-200a-451d-a2c8-e2fd458f0193/graph

/cc @fabpot

aitboudad · 2015-03-27T10:35:16Z

@Tobion can you review this PR ?

GromNaN · 2015-03-28T18:02:32Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml

@@ -152,5 +153,13 @@
        <service id="translation.extractor" class="%translation.extractor.class%"/>

        <service id="translation.writer" class="%translation.writer.class%"/>
+
+        <!-- Cache -->
+        <service id="translation.cache.default" class="Symfony\Component\Translation\MessageCache" public="false">


You can make this service private. Then it could be inlined or removed by the compiler.

Yes it is already. Sorry.

aitboudad · 2015-03-29T22:27:18Z

ping @symfony/deciders

xabbuh · 2015-03-30T20:53:39Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -652,6 +652,10 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder

        $container->setParameter('translator.logging', $config['logging']);

+        if (isset($config['cache']) && $config['cache']) {


Isn't isset($config['cache']) alone sufficient enough?

aitboudad · 2015-03-31T21:08:39Z

any feedback, I really want it to be available in 2.7 :)

xabbuh · 2015-03-31T21:26:36Z

👍

fabpot · 2015-04-02T06:52:57Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml

+
+        <!-- Cache -->
+        <service id="translation.cache.default" class="Symfony\Component\Translation\MessageCache" public="false">
+            <argument key="cache_dir">%kernel.cache_dir%/translations</argument> <!-- cache_dir -->


The key can be removed as it's not used.

stof · 2015-04-02T10:06:24Z

src/Symfony/Component/Translation/Translator.php

-     * @param bool                 $debug    Use cache in debug mode ?
+     * @param string                            $locale   The locale
+     * @param MessageSelector|null              $selector The message selector for pluralization
+     * @param string|null|MessageCacheInterface $cache    The directory to use for the cache


the description is not accurate anymore

fabpot · 2015-04-03T12:23:02Z

I tend to agree with @mpdude

aitboudad · 2015-04-03T13:29:31Z

anyway the main thing here is to provide a doctrine cache for translation, what I didn't like for ConfigCache is it mainly related to resource file.

aitboudad · 2015-04-04T09:45:41Z

any decision about this feature the current implementation look good to me :),
the #14178 look nice but didn't solve the issue at all,
another thing to consider here is that users can use the cache without the config component deps.

mpdude · 2015-04-04T10:12:08Z

#14178 tries to solve a completely different problem.

But even with sticking to the current way of using ConfigCache, we should be able to improve the Translator by possibly having several caches (loading smaller MessageCatalogues) and/or improved memory efficiency by keeping messages in a shared memory cache (which this PR is about, if I understand it correctly).

mpdude · 2015-04-06T19:07:29Z

If I got the code right (not sure about that), your goal is to keep messages in a key-value-store type cache.

That way, the Translator does not have to load a possibly huge MessageCatalogue (or even several of them) from the ConfigCache only to return a single message. It might be able to retrieve the message directly from the key-value-cache.

Also, the message catalogue will mostly be kept in shared memory and reduce the memory footprint for a single PHP process. With the current implementation, the entire Catalogue will be read from the ConfigCache into per-request memory.

So, if this understanding is correct, can't we find a way to make the Translator (key-value-)cache aware and use a lookup strategy based on (locale, domain, message-id) without having to touch the ConfigCache related parts at all?

Here's the steps such a Translator would need to do:

Figure out if the catalogue is still fresh/valid

For this, we could stick with the current ConfigCache validation. If we detect that we need to rebuild the ConfigCache, we subsequently also need to copy/map the Catalogue (after we loaded it) into the key-value-store/cache.
Look into the cache

Before requireing the Catalogue from the ConfigCache, check whether the requested (locale, domain, message-id) is available in the (key-value-)cache. If so, return it from there.
Cache miss

If a requested translation cannot be found, it does either not exist or it may be missing because the key-value-cache has been purged.

We could detect the latter by having an extra key that is written once the cache was loaded. If that key is missing, re-populate the key-value-cache with the MessageCatalogue loaded from the ConfigCache (!).

Does that make sense?

dunglas · 2015-04-06T20:36:28Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -659,6 +659,10 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder

        $container->setParameter('translator.logging', $config['logging']);

+        if (isset($config['cache'])) {


You should prefix cache entries with something specific with the current installation to prevent conflicts if several Symfony apps run on the same server.

The example for the validator: https://github.com/aitboudad/symfony/blob/translation_catalogue_cache/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L774

aitboudad · 2015-04-06T21:28:17Z

@mpdude you understand this topic now better than me :) , below my answers:

If I got the code right (not sure about that), your goal is to keep messages in a key-value-store type cache.

The main think here for me is to move the cache outside of Translator and adding an initial implementation of key-value-store(doctrine/cache).

can't we find a way to make the Translator (key-value-)cache aware

Right I think missed this part even if we use key-value-store we need ``ConfigCache` to make cache aware better.
I work now on this part so I'll fix it soon.

aitboudad · 2015-04-07T08:35:41Z

@mpdude Hi,
If we decide to use the Translator (key-value-) we don't need anymore to save cache in file !! and I think the current limitations of ConfigCache is it doesn't support key/value cache.

So what I made now with 69225b8 is refresh cache when resources file change.

And to understand how I make the Translator (key-value-)cache aware:

1 - Figure out if the catalogue is still fresh/valid.

isFresh

2 - Look into the cache

check whether the requested (locale, domain, message-id) is available in the (key-value-)cache

3 - Cache miss

If that key is missing, re-populate the key-value-cache with the MessageCatalogue loaded from the Loaders and not ConfigCache !

mpdude · 2015-04-07T10:26:46Z

If you're trying to go without ConfigCache, you will end up re-implementing the freshness check it provides (for example here). You also seem to need the DoctrineMessageCache as a replacement for ConfigCache.

My suggestion was to keep the ConfigCache part as-is. It's just a file sitting on the disk and does no harm. That way, you can still use the resource validation logic it provides. You will see from within Translator when the catalogue has to be rebuilt, that's when the cache-is-fresh check fails.

In other words, the DoctrineCache would just be another caching layer on top of ConfigCache: In Translator, before you load the entire Catalogue from disk, see if you can find that particular message in the DoctrineCache first.

When you have the Catalogue at hand anyways (because you had to rebuild it or load it because the message was not in DoctrineCache), also save it to the DoctrineCache.

aitboudad · 2015-04-07T10:39:56Z

@mpdude well I agree to some extent, but I didn't like the idea of writing cache twice in ConfigCache then DoctrineCache.

@fabpot what do you think?

jakzal · 2015-04-21T17:52:00Z

I'd hold off and do not merge this into 2.7. Note that I didn't review this PR in details just yet, and I only scanned through comments. Forgive me if I'm repeating someone else here.

We need to think of how to organise doctrine integration more carefully and make it consistent across the framework. With the current implementation I don't think that doctrine cache classes belong to the Doctrine bridge, as they're only use is the translator.

Serializer and Validator have their own cache implementations, slightly different from each other - first relies on doctrine/cache directly, the later adapts its interface. This PR introduces yet another approach. All I'm saying we should stop and consider if we can make it a bit more consistent.

aitboudad · 2015-05-03T00:47:21Z

The safe way is to split this PR into 2, let's discuss about adding message cache interface and once accepted I'll open another one for doctrine cache.

…ion for validator mapping (jakzal) This PR was merged into the 2.8 branch. Discussion ---------- [FrameworkBundle] Add a doctrine cache service definition for validator mapping | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#5409 Following #12975, this PR only registers a new service so it's possible to use the new doctrine based cache implementation instead of the deprecated one. To use it, the end user would need to configure it in his `config.yml`: ```yaml framework: validation: cache: validator.mapping.cache.doctrine.apc ``` In 3.0 we'll be able to replace the deprecated definition by aliasing `validator.mapping.cache.apc` to `validator.mapping.cache.doctrine.apc`. I thought of automatic wrapping of services which implement doctrine interface, but decided it would be too magic. I'm not convinced if APC is a good default anymore and hope for some discussion. I've used it as it's also used in serializer, and probably translation (see #13986). Since there's a built in opcache in more recent PHP versions, and apcu doesn't seem to be stable, there are better choices. Perhaps a better default would be a filesystem cache (not better performing, but it works anywhere). Commits ------- 0642911 [FrameworkBundle] Add a doctrine cache service definition for validator mapping

aitboudad added the Translation label Mar 19, 2015

xabbuh reviewed Mar 20, 2015
View reviewed changes

aitboudad mentioned this pull request Mar 23, 2015

adjust service definitions to work with symfony 2.6 liip/LiipTranslationBundle#36

Merged

aitboudad force-pushed the translation_catalogue_cache branch from e20e348 to 40f2628 Compare March 26, 2015 14:06

aitboudad changed the title ~~[WIP][Translation] added message cache.~~ [2.7][Translation] added message cache. Mar 26, 2015

aitboudad force-pushed the translation_catalogue_cache branch 4 times, most recently from b39dd88 to 45882cd Compare March 26, 2015 14:42

aitboudad force-pushed the translation_catalogue_cache branch from 45882cd to b17354f Compare March 28, 2015 02:30

GromNaN reviewed Mar 28, 2015
View reviewed changes

xabbuh reviewed Mar 30, 2015
View reviewed changes

aitboudad force-pushed the translation_catalogue_cache branch from b17354f to dfd828f Compare March 30, 2015 21:49

aitboudad force-pushed the translation_catalogue_cache branch 2 times, most recently from d375b6d to 803e7d9 Compare April 1, 2015 09:06

aitboudad mentioned this pull request Apr 1, 2015

[RFC] [Config] Use a Factory for ConfigCache instances #7781

Closed

fabpot reviewed Apr 2, 2015
View reviewed changes

aitboudad force-pushed the translation_catalogue_cache branch from 803e7d9 to eb33493 Compare April 2, 2015 07:00

stof reviewed Apr 2, 2015
View reviewed changes

aitboudad force-pushed the translation_catalogue_cache branch from cbd6c79 to f5c9a3d Compare April 4, 2015 12:44

dunglas reviewed Apr 6, 2015
View reviewed changes

aitboudad force-pushed the translation_catalogue_cache branch from f5c9a3d to af78b62 Compare April 6, 2015 21:50

aitboudad force-pushed the translation_catalogue_cache branch from 69225b8 to 2860af4 Compare April 7, 2015 10:19

aitboudad mentioned this pull request Apr 7, 2015

Some ideas to improve performance of the Translator component #13948

Closed

aitboudad force-pushed the translation_catalogue_cache branch from 2860af4 to 0df4e75 Compare April 9, 2015 23:03

aitboudad mentioned this pull request Apr 11, 2015

[Translation] Revert inlining fallback catalogues as it might cause inconsistent results when a cache is used #14315

Merged

aitboudad added the Feature label Apr 12, 2015

jakzal mentioned this pull request Apr 21, 2015

[FrameworkBundle] Add a doctrine cache service definition for validator mapping #14429

Merged

aitboudad force-pushed the translation_catalogue_cache branch from 0df4e75 to d1b1986 Compare April 28, 2015 14:04

[Translation] added message cache + doctrine cache.

4794d8b

aitboudad force-pushed the translation_catalogue_cache branch from d1b1986 to 4794d8b Compare April 28, 2015 14:13

aitboudad mentioned this pull request May 3, 2015

[2.8][Translation] added message cache. #14526

Closed

aitboudad closed this May 3, 2015

aitboudad deleted the translation_catalogue_cache branch May 3, 2015 00:57

aitboudad mentioned this pull request Sep 30, 2015

[2.8][Translation] added DoctrineMessageCatalogue. #16036

Closed

aitboudad mentioned this pull request Apr 3, 2017

[Translator] move loading catalogue out of Translator. #17563

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.7][Translation] added message cache. #13986

[2.7][Translation] added message cache. #13986

aitboudad commented Mar 19, 2015

xabbuh Mar 20, 2015

aitboudad Mar 26, 2015

aitboudad commented Mar 26, 2015

aitboudad commented Mar 27, 2015

GromNaN Mar 28, 2015

aitboudad Mar 28, 2015

GromNaN Mar 28, 2015

aitboudad commented Mar 29, 2015

xabbuh Mar 30, 2015

aitboudad commented Mar 31, 2015

xabbuh commented Mar 31, 2015

fabpot Apr 2, 2015

aitboudad Apr 2, 2015

stof Apr 2, 2015

fabpot commented Apr 3, 2015

aitboudad commented Apr 3, 2015

aitboudad commented Apr 4, 2015

mpdude commented Apr 4, 2015

mpdude commented Apr 6, 2015

dunglas Apr 6, 2015

aitboudad Apr 6, 2015

aitboudad commented Apr 6, 2015

aitboudad commented Apr 7, 2015

mpdude commented Apr 7, 2015

aitboudad commented Apr 7, 2015

jakzal commented Apr 21, 2015

aitboudad commented May 3, 2015

		@@ -652,6 +652,10 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder

		$container->setParameter('translator.logging', $config['logging']);

		if (isset($config['cache']) && $config['cache']) {

		@@ -659,6 +659,10 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder

		$container->setParameter('translator.logging', $config['logging']);

		if (isset($config['cache'])) {

[2.7][Translation] added message cache. #13986

[2.7][Translation] added message cache. #13986

Conversation

aitboudad commented Mar 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aitboudad commented Mar 26, 2015

aitboudad commented Mar 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aitboudad commented Mar 29, 2015

Choose a reason for hiding this comment

aitboudad commented Mar 31, 2015

xabbuh commented Mar 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Apr 3, 2015

aitboudad commented Apr 3, 2015

aitboudad commented Apr 4, 2015

mpdude commented Apr 4, 2015

mpdude commented Apr 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aitboudad commented Apr 6, 2015

aitboudad commented Apr 7, 2015

mpdude commented Apr 7, 2015

aitboudad commented Apr 7, 2015

jakzal commented Apr 21, 2015

aitboudad commented May 3, 2015