-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[Cache] memcache fix for J3.6.2, memcache(-d) performance improvements, less connections, add support for 'platform specific caching' #11565
Conversation
Looks like this PR reveals a memcache issue with our storage controller and HHVM Look like HHVM doesn't like " assigning values to the array by omitting the key, resulting in an empty pair of brackets ([])." $arr[key] = value;
$arr[] = value;
// key may be an integer or string
// value may be any value of any type If $arr doesn't exist yet, it will be created, so this is also an alternative way to create an array. This practice is however discouraged because if $arr already contains some value (e.g. string from request variable) then this value will stay in the place and [] may actually stand for string access operator. It is always better to initialize variable by a direct assignment. I believe we can fix this by changing line 194 from |
Fix broken memcache storage on J3.6.2, refresh code joomla#11565
I can change it but I think that HHVM do not use
|
From what I can tell the following line is returning a string rather than an array or false. $index = static::$_db->get($this->_hash . '-index'); |
I can not confirm that on my local hhvm. with hhvm 3.14.4 server from (deb http://dl.hhvm.com/ubuntu xenial main):
URLs: works. I have got only: |
I'm just going by the conditions necessary to reproduce the error as found on Travis CI. You can see the mock code at this link https://3v4l.org/HLGlZ You can change $index to an array, null or to false and there is no error. But if $index is a string, the array modify opperator with throw the error. The only conditions for $index to be a string in the cache MEMCACHE store() code is for the following line to return a string $index = static::$_db->get($this->_hash . '-index'); It's possible that it's only returning a string under our PHP unit tests. |
IMHO there is bug in travis tests, or php-memcache is too old. Example which I do not understand:
Why there is memcacheD extension loaded instead memcachE? |
Please correct me if I'm wrong. Why did not travis see that memcachE can not connect to server after applied PR #9622. |
This is telling Travis CI to adjust the PHP environment if the PHP version needs to make special settings for MEMCACHE if [[ $INSTALL_MEMCACHE == "yes" ]]; then phpenv config-add build/travis/phpenv/memcached.ini; fi It's specific to Travis and the container settings and only sets up MEMCACHE in the PHP environment. It's also not required for some PHP versions. In some versions both MEMCACHE and MEMCACHED are supported. Currently both MEMCACHE and MEMCACHED are supported, even though MEMCACHE is outdated and will go away at some future date. (HHVM only supports MEMCACHE and PHP 7 only supports MEMCACHED) |
Looking deeper into what is happening at this line. $index = static::$_db->get($this->_hash . '-index'); Memcache::get returns a string unless it was passed an array of keys. http://php.net/manual/en/memcache.get.php This is why $index is a string when the array modifier opperator tries to append $tmparr to the value. Causing the error. |
I thought that too but when I checked yesterday MEMCACHE is still working on php7.0 - you can test it on ubuntu 16.04 LTS.
IMHO this is a bug/typo, it should be:
I created a PR for that #11576. |
@@ -31,15 +31,15 @@ class JCacheStorageMemcache extends JCacheStorage | |||
* @var boolean | |||
* @since 11.1 | |||
*/ | |||
protected $_persistent = false; | |||
protected static $_persistent = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to make these static. Actually I'd say that's a step backward. The only reason the Memcache instance is static is because Joomla still doesn't believe in proper dependency injection but prefers global singletons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me explain my point of view.
Before start set compression for memcache to true in Global configuration;
Example:
- Component create instance of cache with some options like lifetime=600
- Cache instance get instance of storage:
$hash = md5(serialize($this->_options));
if (isset(self::$_handler[$hash]))
{
return self::$_handler[$hash];
}
self::$_handler[$hash] = JCacheStorage::getInstance($this->_options['storage'], $this->_options);
-
first storage instance is created, compression=1, yes?
-
Next joomla get modules.
-
Module has different lifetime=900 then options is different then joomla create another instance
of storage. -
Now we have to storages of memcache - do you agree?.
-
First storage (before patch) has set compression=1 because getConnection() is called
-
Second storage instance (before patch) has compressio=0 because this does not need to create another connection, (because use existed static::$db)
What we have before patch? Only first instance of storage uses compression.
If you understand above, then
should I change it and each time of call __construct() set
$this->compression = JFactory::getConfig()->get('memcache_compress', false) ? MEMCACHE_COMPRESSED : 0;
Is it better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably won't happen before 4.0 but I'd rather see all the adapters
have the internal connection injected versus us having such a static
singleton based API. We should be able to create a single Memcache(d)
connection and drop that into both the cache and session as an example.
But back on track...
So right now because the connection is static there's little value in these
options being non static if they only affect the connection params.
However if they can affect other behaviors they should be configurable
per-instance. Granted I don't see how this is actually useful with our
current structure but imagine a scenario where different cache data might
be stored on different Memcache(d) instances for whatever reason, each
connection should be configurable basically.
It just feels wrong that whatever ends up being the first call to these
class' constructors dictates the connection for the full request without
getting into some PHP magic (i.e. Reflection or overloading the autoloader
and creating custom classes).
On Saturday, August 13, 2016, Tomasz Narloch [email protected]
wrote:
In libraries/joomla/cache/storage/memcache.php
#11565 (comment):@@ -31,15 +31,15 @@ class JCacheStorageMemcache extends JCacheStorage
* @var boolean
* @SInCE 11.1
*/
- protected $_persistent = false;
- protected static $_persistent = false;
Let me explain my point of view.
Before start set compression for memcache to true in Global configuration;
Example:
- Component create instance of cache with some options like lifetime=600
- Cache instance get instance of storage:
$hash = md5(serialize($this->_options)); if (isset(self::$_handler[$hash])) { return self::$_handler[$hash]; } self::$_handler[$hash] = JCacheStorage::getInstance($this->_options['storage'], $this->_options);
first storage instance is created, compression=1, yes?
Next joomla get modules.
Module has different lifetime=900 then options is different then joomla
create another instance
of storage.Now we have to storages of memcache - do you agree?.
First storage (before patch) has set compression=1 because
getConnection() is calledSecond storage instance (before patch) has compressio=0 because this
does not need to create another connection, (because use existed
static::$db)What we have before patch? Only first instance of storage uses compression.
If you understand above, then
should I change it and each time of call __construct() set$this->compression = JFactory::getConfig()->get('memcache_compress', false) ? MEMCACHE_COMPRESSED : 0;
Is it better?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/pull/11565/files/4241cbcde6f909b613017fdd4dac8ea9d1f64ad2#r74691810,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoUTZolV7LHqcIg9DyR2hsIfSwLGAks5qfihcgaJpZM4JiqOc
.
@mbabker I changed problematic stuff. |
{ | ||
$index = array(); | ||
} | ||
$index = $index ? $index : array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if changing to a ternary operator is a good design choice here. The ternary operator in PHP does a copy before evaluating the statement, This results in a performance hit as a data set being evaluated grows (i.e. the more data in the $index variable the slower the ternary operator evaluation is)
Personally, I avoid using ternary operators with objects or arrays since you risk the performance hit from the variable copy it does before evaluation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tip.
I have removed ternary and rebased to the latest staging to resolve the conflict. |
@csthomas Would you change line 190 in the I've submitted a PR to your branch for this change. |
Is the more issue there? Can I ask for test. Test should be simple: check whether cache works or not, test clear cache, etc. |
Finding people with access to certain PHP configurations is challenging at times. I have MemcacheD set up on my local system but not Memcache and I don't have any servers running the latter to test this against. |
If you have access to root account you could run: |
May be someone with HHVM can test, memcache extension is probably built in. |
Example which I use for hhvm on kubuntu 16.04 LTS.
|
I have tested this item ✅ successfully on 3aa94f5 This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11565. |
|
||
if ($servers) | ||
{ | ||
if ($servers[0]['host'] != $host || $servers[0]['host'] != $port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can combine that line with the if one above (&&) and avoid one more level of nesting.
Also is this correct $servers[0]['host'] != $port
?
Thanks @yvesh for code review. After my commit I probably still need 2 success tests. |
Maybe a stupid question but we have Memcached running on our servers and on our Joomla 3.6.2 websites without any problems (PHP 5.6.24 with cloudlinux) So what do we need to fix which works for us? |
This PR only add improvement for memcacheD But you can not use MemcachE on J3.6.2 |
If There are the same changes in both, except @photodude @mbabker Can I ask you to to make a test? |
I have tested Memcached ✅ on PHP 5.6.24/Cloudlinux/Mariadb/cgi-fcgi on Joomla 3.6.2 successfully This runs well on a Joomla 3.6.2 multi lingual testsite gwsdev2.work. I also tested this on the same site with PHP7 enabled and works fine as well Short remark on Memcache. Memcache is not available yet for PHP7. This is the reason why we have removed it from the PHP-extensions in the PHP-selector of Cloudlinux altogether and for now just provide Memcached. This is not any issue since in the CL environment you get OPCache and that works just so nice and fast in combination with Memcached |
Thank you @gwsdesk.
Official not, but at least one linux distribution have memcache working with PHP7. If you have Ubuntu 16.04 on some machine then you can install php-memcache (PHP7) from default repository. There is probably unofficial version of memcache compiled in deb. |
Thanks Thomasz but Cloudlinux does not support Memcache in PHP7 yet. In On 8/25/2016 3:33 PM, Tomasz Narloch wrote:
|
Can your test can be mark as successful at https://issues.joomla.org/tracker/joomla-cms/11565 If you have a few minutes You can test memcache as follow: |
I have tested this item ✅ successfully on 4e5e96b This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11565. |
I have tested this item ✅ successfully on 4e5e96b As soon as I applied this revised patch I got the following notice
I assume this warning was due to an old cache state; as it went away after turning cache off and then back on in Global settings. Since it was a transient message and resolved after resetting the cache I don't think there is anything to do but note the message. This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11565. |
@photodude this means that it is not successful since it shows the original error (which goes away if you disable caching or change to file. Please remove the 'successful' tag and replace with 'unsuccessful' ? ❌ |
I don not know @photodude steps but I assume he enabled memcachE before applying patch. Besides above. |
Here is what I had done. @gwsdesk
As you can see the warning was due to the inconsistent state the cache was in between reapplying this patch after it was updated. I stand by my choice in marking it as a successful test. |
@photodude I read it correctly now and only noticed now your initial test as well. Thanks for successful testing ;-) |
@rdeutz can this be marked RTC now? |
@photodude yes it can This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11565. |
@joomla-cms-bot can you please help here? Thanks 😄 |
Pull Request for Issue (Not yet)
Summary
Cache storage memcache from J3.6.2 is not working.
This PR fix that.
I also add the same changes to memcached.
This is part of another (more optimized) PR #10565.
Summary of Changes
[UPDATED]
Before only first instance of memcache can set compression to true on
getConnection
method.Next instances did not call
getConnection
and then did not use compression at all.Move
$this->_compress = ...
to consctructor.https://github.com/joomla/joomla-cms/pull/11565/files#diff-8104780960bee59ef8a377ffd818797aR47
Check
static::$_db === null
beforeisSupported()
.If connection exists then skip call
isSupported
and do not create useless connections.https://github.com/joomla/joomla-cms/pull/11565/files#diff-8104780960bee59ef8a377ffd818797aL55
Fix persistent connection. Remove test connection which break persistent.
https://github.com/joomla/joomla-cms/pull/11565/files#diff-8104780960bee59ef8a377ffd818797aL79
Remove race condition. We do not need to initiate empty list.
https://github.com/joomla/joomla-cms/pull/11565/files#diff-8104780960bee59ef8a377ffd818797aL89
Joomla uses only one memcache server. Remove
replace
and use onlyset
method.https://github.com/joomla/joomla-cms/pull/11565/files#diff-8104780960bee59ef8a377ffd818797aL194
Fix
remove
method. Search in all list instead of in first item only.https://github.com/joomla/joomla-cms/pull/11565/files#diff-8104780960bee59ef8a377ffd818797aL236
Do not test connection in
isSupported
method https://github.com/joomla/joomla-cms/pull/11565/files#diff-8104780960bee59ef8a377ffd818797aL311Do not lock twice. Do not call
lockindex
forlock
/'unlock' method.https://github.com/joomla/joomla-cms/pull/11565/files#diff-8104780960bee59ef8a377ffd818797aL334
https://github.com/joomla/joomla-cms/pull/11565/files#diff-8104780960bee59ef8a377ffd818797aL395
Fix
locklooped
.https://github.com/joomla/joomla-cms/pull/11565/files#diff-8104780960bee59ef8a377ffd818797aR375
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/cache/controller/callback.php#L99
_getCacheId($id, $group)
for memcache(-d).If
Platform specific caching
inGlobal Configuration->Cache Settings
is enabledthen mobile (user agent) user without that patch could not delete/see all cache data.
Testing Instructions
(Optional)
Check how many connection (in memcached stats) generates joomla before patch and after patch.
After patch there should be less connections.
Other method to test.
#11565 (comment)