symphonists / cacheabledatasource

[Symphony 2] Cache the XML output of data sources to improve performance
http://symphonyextensions.com/extensions/cacheabledatasource/
Other
10 stars 13 forks source link

Performance issue in big systems #11

Open michael-e opened 10 years ago

michael-e commented 10 years ago

In a "big system" (a bigger-than-usual Symphony web app) I experienced a surprising issue. Although I am pretty sure that there is no simple solution to this, I am posting it here for reference.

It's a performance issue! Well, not in the frontend (which is about twice as fast with most of the DSs cached), but upon saving entries. The extension will attempt to delete a lot of cache files (in the flushCache function), using PHP's glob() to find all of them. I have more than 50.000 cache files, and even on my fast dedicated server each glob() will take a bit more than 40 ms. Counting 19 datasources for one section and 5 "associated" sections, the extension will perform 95 globs. So it takes more than 4 seconds to delete the cache files.

I played with this issue for a whole day, and I haven't found any workaround. Reducing everything to a single glob call, for example, by adding all the datasource handles to an array first, then doing s.th. like

$cache = glob($cacheDir.'{'.implode(',', $flushHandles).'}_*.xml', GLOB_BRACE);

doesn't help at all—it just doesn't work any faster.

In my eyes the only solution would be an asynchronous/background process, which would mean a major rewrite of the extension. If somebody has a different idea, please post it here.

creativedutchmen commented 10 years ago

If somebody has a different idea, please post it here.

I do. I'm currently doing a rewrite of this extension, as it's quite buggy on later versions, and on these versions the core provides ways of using other caching backends (redis, memcached, xcache, etc).

When I'm done with this, you should be able to use redis (which I think you're already using) as a backend, which will eat through 50.000 entries in no time.

michael-e commented 10 years ago

Cool, that sounds very promising!

nitriques commented 10 years ago

+1 Indeed, but I would really wish that caching extensions would not rely on a specific technology and could request a cache provider and work it and abstract class...

creativedutchmen commented 10 years ago

Got a 70% working version running locally now. Unfortunately the cache providers are not as powerful as hoped, so it currently depends heavily on the caching backend to "do the right thing".

Will try to get the remaining 30% done by tomorrow, as this might be the impulse the implementation needs to receive some love.

On Fri, Aug 15, 2014 at 3:27 AM, Nicolas Brassard notifications@github.com wrote:

+1 Indeed, but I would really wish that caching extensions would not rely on a specific technology and could request a cache provider and work it and abstract class...

Reply to this email directly or view it on GitHub: https://github.com/symphonists/cacheabledatasource/issues/11#issuecomment-52265487

creativedutchmen commented 10 years ago

For those interested, the 70% version I spoke of can be found in my fork: https://github.com/creativedutchmen/cacheabledatasource/tree/cache_providers.

There are a few things I had to work around, such as that there's no way to invalidate a block of hashes at once. So flushing all datasources loading from a specific section requires a workaround which is heavily dependent on how well-behaved your caching backend is.

In my branch this bit is not yet implemented per-se, but the foundation: https://github.com/creativedutchmen/cacheabledatasource/blob/cache_providers/extension.driver.php#L78-L91 is there.

Also, at this moment the TTL is completely ignored. Just so you know ;)

creativedutchmen commented 10 years ago

+1 Indeed, but I would really wish that caching extensions would not rely on a specific technology and could request a cache provider and work it and abstract class...

That's exactly what I am doing here. No more baked-in tech, but the core classes that are backend-agnostic.

michael-e commented 10 years ago

@creativedutchmen, have you finished your work on this in the meantime?

creativedutchmen commented 10 years ago

Unfortunately, no. I encountered show-stopper problems with the caching API, which basically means there's no way to do it without being very future-unfriendly...

nitriques commented 10 years ago

which basically means there's no way to do it without being very future-unfriendly

I would not mind it...

creativedutchmen commented 10 years ago

I would not mind it...

Pretty sure you would mind. The Caching API currently supports switching caching providers (memcached, redis, apc to name a few). But currently the exposed methods there aren't sufficient for what I'd need for this extension.

So, when I was going to implement this anyway, I would write it with a specific provider in mind (redis, most likely). So if you wanted apc, you'd be out of luck...

nitriques commented 10 years ago

So if you wanted apc, you'd be out of luck...

I do not understand why....

But currently the exposed methods there aren't sufficient for what I'd need for this extension.

Yeah that's why we should build new interface because right now, it's mess.

creativedutchmen commented 10 years ago

Yeah that's why we should build new interface because right now, it's mess.

Yes. Unfortunately I don't have the time to take on this (rather big) project at this moment.

I do not understand why....

Because the API is unusable, and I don't have time to completely reinvent it. Of course, if we wait for the API to improve then this extension can use it.

nitriques commented 10 years ago

Ok, now I understand, thanks.

michael-e commented 10 years ago

So I guess I have no luck with this at the moment. I had to disable my Cacheable Datasource extension again, because of the problems mentioned above. I have some events with "allow multiple" enabled, and people save 20 entries or more. In this case the problem above is really serious, because the triggering delegate is called after each successful save. The PHP process timed out...

michael-e commented 10 years ago

And isn't it strange that the extension supposed to do some caching fails completely if you really need it (i.e. in a big system)?

creativedutchmen commented 10 years ago

And isn't it strange that the extension supposed to do some caching fails completely if you really need it (i.e. in a big system)?

Very. But using the disk is not the best solution in the first place. The main reason behind this is that everything had to be implemented from scratch, and the disk is easy and good enough in most cases.

That said, we really need to improve the caching API. I don't mind helping there, but I can't afford to do all the work.

michael-e commented 10 years ago

As an interim solution, I think about writing a custom event for the "save multiple" case (because currently it is even slow without the Cachebale DS entension, ad all I want to do is save some "entry order" numbers, nothing else). In this case I could call the triggering delegate only once, after having done the work.

Solution 2: If I stick to the native Symphony "allow multiple" event logic, I might hack an additional delegate into Symphony, which is triggered only once after doing either "save" or "multiple save".

What would you do, @creativedutchmen? Solution 1, right?

michael-e commented 10 years ago

FYI: Regarding the "slow saving of entry order numbers", I did some research and found a culprit in the way I save the order of entries from my frontend administration interface. Basically I am an idiot, using the Order Entries field incorrectly. The extension is designed to save only one entry order number and do the rest with a special database query like UPDATE tbl_entries_data_{$this->get('id')} SET value = (value + 1) WHERE value >= ".$data. Now if you—like me—save 60 or more entries using "allow multiple", each of the entries having an entry order field value (starting from 1), this database query will be performed for each of the entries. Having more than 17,000 entries in the section, the query will be very slow, like 300 or 350ms.

350ms/entry * 60 entries = 21s

Given the fact that the order of entries is never changed in the Symphony backend (but only from the frontend interface) I should really use a different field (like the number field). I actually don't need to update all the other section entries, because in my application the entries that are actively saved are a content unit on its own. In other words: "Position number collisions" in this section are no issue; all I need is correct numbers for the entries which are currently saved.

This is the first thing that I will have to change, then think about the caching/delegate stuff.

nitriques commented 10 years ago

Wow Michael, this is some nice infos you are sharing here. Thanks!

michael-e commented 10 years ago

You're welcome!

michael-e commented 10 years ago

FYI: I have a plan how to move to the number field. But even that won't be enough. According to my tests, it will still take more than 20s to save position numbers for 60 entries. The reason is in the Symphony core, which (re-)commits each entry completely even if only one field is POSTed. (And these entries have some more fields…)

So I would like to hack that (and use some simple database queries like:

UPDATE `sym_entries_data_1206` SET `value` = 1 WHERE `entry_id` = 239490;

to just save the numbers). I would like to keep all the functionality like event filters etc. (the filters performing security tasks, for example), but at the moment I don't see any elegant solution to just change commiting of the entry without replicating at least the whole __doit function which is in /lib/toolkit/events/class.event.section.php.

Ideas would be highly appreciated.

(Once that is done, I will have to think about the caching issue again, of course.)

brendo commented 10 years ago

The reason is in the Symphony core, which (re-)commits each entry completely even if only one field is POSTed.

The only reason I can think of that is so other fields can update appropriately, such as the Reflection field. It's a nice find though, I think it's by accident that it occurs!

Perhaps a filter could be created that allows isolated event updates?

michael-e commented 10 years ago

Perhaps a filter could be created that allows isolated event updates?

I don't really see what you have in mind here. How could that work?

michael-e commented 10 years ago

I don't think that we should touch Symphony's "get-the-complete-entry-and-commit-everything" behaviour when updating entries, because that would surely break a lot. I will solve my edge case with custom code, and I have an idea how to do it. I will report when it's done.

michael-e commented 9 years ago

Still I haven't found any solution.

I have a plan how to move to the number field.

Done.

But even that won't be enough. According to my tests, it will still take more than 20s to save position numbers for 60 entries.

Confirmed.

The reason is in the Symphony core, which (re-)commits each entry completely even if only one field is POSTed.

Not sure if that is the full truth. As far as I see, the Symphony admin (backend) does the same thing if you use the with-selected menu on multiple items, i.e. every entry gets committed. However, saving 60 to 70 entries takes more than 20s from the frontend and less than 2s in the backend. How can that be?

michael-e commented 9 years ago

Answering my own question: It's the (event) filters!!!

I will try and find out more.

michael-e commented 9 years ago

Coming closer: Slowness is caused by one of my custom (security-related) event filters. If I remove this event filter, saving all the entries from the frontend takes 3 to 4 seconds (instead of more than 20 seconds).

Now I have to find out what exactly takes so long in this filter… (Removing it is no option.)

michael-e commented 9 years ago

FYI: LOL, found the "slow saving" issue. My bad. I will report later (cause I am in a hurry).

brendo commented 9 years ago

Glad to hear there's a resolution :)

michael-e commented 9 years ago

FYI:

As stated earlier, I improved performance even with the native "allow multiple" event by using the number field. But still things were rather slow, so I searched systematically by commenting/un-commenting delegates and functions in the system.

After having found the culprit, all the entries get saved in 3 to 4 seconds, only slightly slower (if at all) than in the Symphony backend. The problem was:

For special access control tasks, I use a custom function hooked into the EventPreSaveFilter delegate. Here I need to get all entry objects to apply my custom logic. So when the delegate gets triggered, my function loops over all the entry IDs and fetches the entry objects. What? Indeed, when I wrote that code, I didn't notice that the delegate itself gets triggered for every entry, so instead of x entries I was pulling x^2 entries from the database. e.g. 60*60 (=3600) entries when only 60 entries are saved. That must be slow...

So this loop (which used the posted entry IDs) was a major mistake on my side, which — interestingly enough — was never noticed until one of my users had a larger number of entries to save. :-)

Remark 1:

In Symphony's event logic, the entry object isn't available for a preSaveFilter, but I noticed that this might change in the future:

// If the `$entry_id` is provided, check to see if it exists.
// @todo If this was moved above PreSaveFilters, we can pass the
// Entry object to the delegate meaning extensions don't have to
// do that step.

(https://github.com/symphonycms/symphony-2/blob/integration/symphony/lib/toolkit/events/class.event.section.php#L329-L332)

(This might improve performance a bit in a case like mine, but it's by far not as essential as my programmatic mistake. Getting an entry object is actually rather fast.)

Remark 2:

All this doesn't solve the inital issue of this thread, of course.

jonmifsud commented 9 years ago

@creativedutchmen did you manage to get around using the Cache Providers or it's still at 70% status? Was planning to eventually get to it as working on a project so I might have a go at it later this month/early next.

nitriques commented 7 years ago

@michael-e was this ever fixed ?

creativedutchmen commented 7 years ago

Whoops, completely missed the updated in this thread, sorry.

I have not continued my work on this, interesting as it is. @michael-e have you?

michael-e commented 7 years ago

I only fixed it in my "big system" by deleting stale cache files with a cron job, executed every minute:

find /var/www/the-big-system/manifest/cache/data-sources -type f -mmin +5 -print0 | xargs -0 rm -f

This guarantees that the number of cache files never grows too large.

I do not think that there is any other solution to this issue (apart from using Redis or s.th. else that is really fast, faster than the filesystem).

jonmifsud commented 7 years ago

@nitriques / @michael-e - I'm actually using cache providers on a fork with a DB Cache (though that potentially has it's own issues/bottlenecks) but works with InnoDB tables and some adjustments with the flushing of the DB cache (as it could cause lock tables there too which brings to a halt)

I've had something with memcache cooked up a while ago which we should deploy on production within the next couple of weeks. I'll keep you posted

nitriques commented 7 years ago

Great, good! @michael-e I do the same with the cachelite extension ;)

michael-e commented 7 years ago

:-)

wdebusschere commented 6 years ago

I confirm this issue, have a section with only 2000 records (with 2 SBL fields), saving a record on a frontend page with ajax, takes some seconds, without this caching, immediately.

jonmifsud commented 6 years ago

@wdebusschere - I suggest trying out what @michael-e did having things clear via a cron task. I'm using a memcache version in production which doesn't delete files and I actually fall-back to database caching when that's not available as the infrastructure I have that seems more feasible than loading up files.

nitriques commented 6 years ago

@wdebusschere @jonmifsud I've created a cron strategy for cachelite, maybe we would need to do the same here.

jonmifsud commented 6 years ago

We’re currently working on a version which should include threading or process forks in situations where you may still want to serve cached content would make sense to reassess then

On Fri, Apr 27, 2018 at 5:48 PM Nicolas Brassard notifications@github.com wrote:

@wdebusschere https://github.com/wdebusschere @jonmifsud https://github.com/jonmifsud I've created a cron strategy for cachelite, maybe we would need to do the same here.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/symphonists/cacheabledatasource/issues/11#issuecomment-385011498, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0ef7STlxpUjp6Hh80QTZfvZhMz80XEks5tsz17gaJpZM4CFXkP .

-- JONATHAN MIFSUD CHIEF STRATEGIST jonathan@maze.digital +356 9983 0394

nitriques commented 6 years ago

Great.