Skip to content
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

Don't use comments? #161

Open
spinitron opened this issue Jul 29, 2018 · 9 comments
Open

Don't use comments? #161

spinitron opened this issue Jul 29, 2018 · 9 comments

Comments

@spinitron
Copy link

It's not ideal that this relatively small dependency dictates opcache.save_comments=1 at the web server config level.

I think you could refactor the parseXyz() methods and @marker comments that are organized into classes and traits into \Closures and strings organized into assoc arrays.

What do you think?

@cebe
Copy link
Owner

cebe commented Jul 29, 2018

does opcache.save_comments=0 really safe some resources or improve speed? In general composition of markdown features could be implemented in a different way, but I do not think it is really necessary.

@spinitron
Copy link
Author

spinitron commented Jul 30, 2018

Yes, it saves opcache shared memory. The memory load for each PHP file is reduced by the number of bytes of comments in the file, iiuc. I agree that it's not necessary to implement without reflecting on comments but I think it's desirable.

We cannot in general quantify the benefit of setting opcache.save_comments=0 since it depends entirely on the project. But I can offer some general qualitative observations...

  • Lots of comments is rather common these days. For example, the libs we depend on often document themselves entirely with comments. So it could be a significant fraction of total SHM demand.
  • Your README offers performance as a selling point while it impacts the SHM load of all code in that opcache.
  • I assume opcache offers the config option because it can yield real operational benefit.
  • Portable, runtime-independent code is nice.
  • Comment reflection is a bit, how should I put it diplomatically?, not what you might expect a modern software textbook to recommend as "good practice".

@spinitron
Copy link
Author

Fwiw, OPcache stats from my servers currently show

  • number of scripts 1430
  • 11 of them under cebe/markdown
  • 36.3 MiB without comments
  • 39.6 MiB with comments, about 9% more

Not a huge difference but it's not insignificant.

@spinitron
Copy link
Author

spinitron commented Nov 3, 2018

Nothing much changed. Detailed opcache status below shows server A and B have almost identical opcache use (no surprise, they have almost identical load). The only important difference is used_memory which is ~10% more with with comments. (This opcache has been up for just over a month.)

stat A B
opcache_enabled true true
cache_full false false
restart_pending false false
restart_in_progress false false
memory_usage
used_memory 39580576 43352104
free_memory 77943056 73007632
wasted_memory 16694096 17857992
current_wasted_percentage 12.43807077407837 13.305240869522095
interned_strings_usage
buffer_size 8388608 8388608
used_memory 3365656 3353072
free_memory 5022952 5035536
number_of_strings 60879 60802
opcache_statistics
num_cached_scripts 1614 1613
num_cached_keys 2225 2226
max_cached_keys 16229 16229
hits 2904881093 3074112339
start_time 1535933633 1535720837
last_restart_time 0 0
oom_restarts 0 0
hash_restarts 0 0
manual_restarts 0 0
misses 99128 160821
blacklist_misses 53734850 60452796
blacklist_miss_ratio 1.8161549426196841 1.928487429157587
opcache_hit_rate 98.18049468407227 98.0663822659208

@cebe
Copy link
Owner

cebe commented Nov 4, 2018

Thank you for the detailed stats!

@Ayesh
Copy link

Ayesh commented Jan 15, 2020

I'm interested in to see if there is any development in this issue.

Ubuntu, Debian and oenrej/php PHP packages have default configuration opcache.save_comments=1, but I personally turn it off save some opcache space. This is my only blocker that prevents me using this awesome package.

Thank you.

@cebe
Copy link
Owner

cebe commented Jan 24, 2020

If you want to use this lib without comments, you could extend the markdown flavor class you want to use and override the following method:

markdown/Parser.php

Lines 254 to 285 in eeb1bf6

/**
* Returns a map of inline markers to the corresponding parser methods.
*
* This array defines handler methods for inline markdown markers.
* When a marker is found in the text, the handler method is called with the text
* starting at the position of the marker.
*
* Note that markers starting with whitespace may slow down the parser,
* you may want to use [[renderText]] to deal with them.
*
* You may override this method to define a set of markers and parsing methods.
* The default implementation looks for protected methods starting with `parse` that
* also have an `@marker` annotation in PHPDoc.
*
* @return array a map of markers to parser methods
*/
protected function inlineMarkers()
{
$markers = [];
// detect "parse" functions
$reflection = new \ReflectionClass($this);
foreach($reflection->getMethods(ReflectionMethod::IS_PROTECTED) as $method) {
$methodName = $method->getName();
if (strncmp($methodName, 'parse', 5) === 0) {
preg_match_all('/@marker ([^\s]+)/', $method->getDocComment(), $matches);
foreach($matches[1] as $match) {
$markers[$match] = $methodName;
}
}
}
return $markers;
}

if you hardcode the array of markers and function names it should all work with opcache.save_comments=0.

I might add some docs about it and possibly also add a configuration option for it...

@cebe
Copy link
Owner

cebe commented Oct 7, 2020

this can be solved in PHP 8 by using attributes: https://wiki.php.net/rfc/attributes_v2

@spinitron
Copy link
Author

@cebe, yes I'm sure it can. It can be solved also with conventional programming.

But the issue with opcache is actually small enough. Allocate enough memory to opcache and stop worrying about it.

In my case the problem has gotten smaller with time as I use fewer comments. With type hints and good names for functions and parameters, comments can be redundant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants