zendtech / ZendOptimizerPlus

Other
915 stars 142 forks source link

Added zend_optimizerplus.max_cached_filesize #42

Closed iliaal closed 11 years ago

iliaal commented 11 years ago

This option allow setting the maximum size of the file to cache, default value is 10MB.

Additionally there are some fixes to allow compilation of the extension directly into PHP and building the extension inside a separate directory.

zendtech commented 11 years ago

Hi Ilia,

I committed thirst two of your commits (the first one had to be modified a bit).

I need to think about the actual "max_cached_filesize" patch. If it's possible to save additional stat() call. I also, didn't understand the following two lines.

+extern zend_module_entry ZendOptimizerPlus_module_entry; +#define phpext_ZendOptimizerPlus_ptr &ZendOptimizerPlus_module_entry

Please explain.

Thanks. Dmitry,

laruence commented 11 years ago

+#define phpext_ZendOptimizerPlus_ptr &ZendOptimizerPlus_module_entry generally, this is used when the extension is built staticlly into PHP, then PHP will use the phpext_modulename_ptr instead of calling to get_module().

laruence commented 11 years ago

if without this, I think you can not build ZendOptimizer+ into PHP staticlly

zendtech commented 11 years ago

Static linkage doesn't work even with these lines. Building as a DSO from common tree works without these lines (but then O+ can't be loaded).

laruence commented 11 years ago

hmm, didn't verify that, but that exactly is what "php_ext_modulename_ptr" used for.

iliaal commented 11 years ago

That's exactly what it is for, without it static build fails with undefined symbols. On 2013-02-25 9:54 AM, "Xinchen Hui" notifications@github.com wrote:

hmm, didn't verify that, but that exactly is what "php_ext_modulename_ptr" used for.

— Reply to this email directly or view it on GitHubhttps://github.com/zend-dev/ZendOptimizerPlus/pull/42#issuecomment-14046439.

zsuraski commented 11 years ago

Ilia,

Can you explain the use case for limiting the maximum file size? It sounds to me that actually if you have a file that large, you'd definitely want it cached. Instead, it gets silently dropped from the cache.

I'm not saying we should encourage people to create such ridiculously large PHP files, but I can't think of a situation where you'd want to exclude a file based on its size. Pointing out specific files you don't want cached makes more sense, which is something you can do with the blacklist mechanism. FWIW, I just learned today that a similar feature in APC 'bit' our ZF team and prevented them from properly loading a classmap into memory, which pushed performance down sharply as the map reached a certain size...

If we really need this - I think it should default to 0 (no limit), but I'm still at the 'if' stage...

iliaal commented 11 years ago

Zeev,

The patch is relavent in a few scenarios, in one case you may have a big configuration file that is used in rare instances and you don't want to load it into cache to save on memory, especially when this file may change frequently and/or have a dynamic name. For example a giant pre-cache file, that once executed would load the relavent data sets into memory and would not be used again.

I don't really see an issue with setting the default value to 0, I guess people who need ti could always set it to a different value.

dstogov commented 11 years ago

Actually, O+ supports "blacklist" file that is more suitable and flexible.

zsuraski commented 11 years ago

Yeah, I agree with Dmitry - I think specifically designating which files you don't want included makes more sense here. The very same large configuration file could actually be a file that you use frequently instead of rarely, and then excluding based on size would backfire...

iliaal commented 11 years ago

It is not necessarily the same file, the name could be generated randomly or be placed in many different locations if the file is per-application and there many instances of app on the machine. That said, if the limit is set to 0 by default as Zeev had suggested there is really no harm or overhead that I can see with this patch being part of O+

nikic commented 11 years ago

@iliaal Too many configuration options is an overhead too. Not for APC, but for the user :)

If those files are autogenerated, then they can probably have some common pattern for the name, so one should be able to match them with a regular expression, or?

iliaal commented 11 years ago

Yes and No, being able to filter by size is useful, I think. The example I gave was just one possible example. Another could be to avoid rarely used files that maybe large to save memory or for ISP to limit the size to just the small files so that more people can benefit from this functionality.

iliaal commented 11 years ago

Dmitry, the issue with a file base filter is that it requires an INI change every-time a new instance of application is deployed and that may not always be possible for the user.

dstogov commented 11 years ago

Ilia, I see that in some specific cases this option may be useful and having default value "0" won't affect performance. I think we may include it a bit later.

iliaal commented 11 years ago

I'll update the patch, I've revised it with rasmus' patch to avoid the stat call. On 2013-02-27 12:25 PM, "Dmitry Stogov" notifications@github.com wrote:

Ilia, I see that in some specific cases this option may be useful and having default value "0" won't affect performance. I think we may include it a bit later.

— Reply to this email directly or view it on GitHubhttps://github.com/zend-dev/ZendOptimizerPlus/pull/42#issuecomment-14186800 .

dstogov commented 11 years ago

Added zend_optimizerplus.max_file_size (default "0"). It allows exclusion of large files from being cached. By default all files are cached.