Closed (fixed)
Project:
Version Control API -- Git backend
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
7 Jun 2009 at 14:37 UTC
Updated:
3 Jan 2014 at 00:29 UTC
Jump to comment: Most recent
The current cache is hacked-together and doesn't work with a restrictive include_path - it probably should be replaced by something like the phpBB Group introduced in their svn for the phpBB 3.0.6 Release, a description can be found here: http://wiki.phpbb.com/Cache
Their code is GPLv2, so we could probably reuse the most important low-level functions.
Comments
Comment #1
sdboyer commentedDammit, I responded but it lost the freakin comment. Don't have the energy to retype the full thing.
Short version: I very much doubt it's worth it to implement our own caching layer. The marginal benefit is significantly outweighed by the cost of being a bad drupal citizen.
cache_get()andcache_set()work fine.Comment #2
CorniI commentedFor a bigger repo I got serious performance improvements here because we use one array which flags if we have recorded a revision yet or not. I'll look out to benchmark cache_get/cache_set against my implementation again, but we really need something else. We also could split out the other cache into another module where it would fit better...
Comment #3
sdboyer commentedSorry, I really shoulda clarified:
So, using temp files, especially ones that contain php code and are being included in, is awful. In fact, it's a security risk to include php files that are webserver-writable. So I'm strongly opposed to the current method. But our current method does _not_ involve cache_get/set - it's already a custom implementation, and based on a quick glance at it, quite a bad one. cache_get/set will be big improvements.
My point with respect to using cache_get/set if at ALL possible, though, is that the system you linked to doesn't have any significant benefits over drupal's caching system with the exception of pluggability, and that's a problem that's being worked on. And being a good drupal citizen is worth trading off some of the performance benefit we might get from a custom implementation (if only because it makes our code more maintainable). The performance improvements to switching to the native drupal system, or at least something that doesn't use php inclusion, should be on par with whatever system you're working with, unless you're doing something that's _highly_ specialized. Which I hope you haven't, simply because of the maintainability issue.
Comment #4
CorniI commentedneh the current drupal cache isn't really suited for our needs for caching and is an awful lot slower than the current hacked one. I'll post some benchmarks when I get some time. It's just that drupal's system is database-driven, and the proposed one file-driven. Just wait for the benchmarks :)
Comment #5
sdboyer commentedI'll be interested to see the benchmarks. But I can tell you right now, the deciding factor is probably going to be code complexity, not the benchmarks, unless they show an order of magnitude's difference over cache_set/get. Because...
Right. For now. But as I said in my last comment, there's a huge effort to make it pluggable right now, at which point it would be CLEARLY preferable to use the drupal system. So unless we're talking about serious UX differences as a result of the performance differences and/or the implementation is very simple and easily ported to the pluggable system later, I'd prefer to not to reinvent the wheel again.
hah. listen to me. 90% of the time I'm the one arguing for MORE file-based caching, configs, etc., in drupal...
Comment #6
CorniI commentedI thought about it again, and got some input by dww(not really related to this, but anyways) changed my mind. I'll soon commit code which removes this cache and just, plain simple uses drupal's cache infrastructure :)
Comment #7
CorniI commentedfixed in 81436387d66104877de90857e8e41273ab9cea08