Port garbage_stats extension into a new xdebug feature. #360

beberlei · 2017-07-08T09:57:20Z

Configuration is done through INI settings:

xdebug.gc_stats_enable=1 Enables the collection of Garbage collection stats
xdebug.gc_stats_output_dir=/tmp
xdebug.gc_stats_output_name=gcstats.%p

Currently the file format is only human readable (tabular), a machine readable version (JSON) may be following later.

Collected | Efficiency% | Duration | Memory Before | Memory After | Reduction% | Function
----------+-------------+----------+---------------+--------------+------------+---------
    10000 |    100.00 % |  0.00 ms |       5539880 |       579880 |   -10.47 % | bar
    10000 |    100.00 % |  0.00 ms |       5540040 |       580040 |   -10.47 % | Garbage::produce
     4001 |     40.01 % |  0.00 ms |       2563048 |       578968 |   -22.59 % | gc_collect_cycles

You can get the concrete filename at runtime with xdebug_get_gcstats_filename().

You can also start the gcstats from within the PHP userland with xdebug_start_gcstats() and stop it with xdebug_stop_gcstats().

Documentation PR here xdebug/xdebug.org#45

derickr · 2017-07-09T14:09:55Z

Should gc_runs be a global zval or demoted to HashTable?

In general, I avoid using PHP's HashTables in Xdebug, but instead I use xdebug_hash functions. They are a bit lighter.

Should there be an INI hook for gc_stats_enable to initialize and destruct the gc_runs, this would avoid the memory allocation and allow users to reset the run data by diabling/enabling the run.

I don't think this is needed. What I would like is to change ```xdebug_gc_stats()toxdebug_get_gc_stats()``, with an argument to allow the wiping of what is already in the hash. This is similar to what https://xdebug.org/docs/all_functions#xdebug_get_collected_errors does.

Is the xdebug.gc_show_report=1 functionality ok or should we write this information to a file instead like traces? This would make it usable in a Web-Request.

I think it should do the same as what the profiler does. Allow the configuration of an "output dir" (a la https://xdebug.org/docs/all_settings#profiler_output_dir and https://xdebug.org/docs/all_settings#profiler_output_name) and add a function to get the filename (a la https://xdebug.org/docs/all_functions#xdebug_get_profiler_filename). I think for Xdebug 3, I want to consolidate all the *_output_dir settings though into one (xdebug.output_dir); but I'm happy to break that issue myself when I do that.

Perhaps it also makes sense to have a parsable output instead of a space-indented output as well? Like https://xdebug.org/docs/all_settings#trace_format ?

Documentation, where to put it in the xdebug.org repo? I haven't grasped the structure from the first gloss over

The feature should go to: https://github.com/xdebug/xdebug.org/blob/master/html/docs/include/features.php, including what it is, how to use it, and why you want to use this. New functions to https://github.com/xdebug/xdebug.org/tree/master/html/docs/include/functions and new settings to https://github.com/xdebug/xdebug.org/blob/master/html/docs/include/settings.php I think a new addition to https://github.com/xdebug/xdebug.org/blob/master/html/docs/include/basic.php#L12 (for FUNC_GC_STATS) is also needed. But write the docs, and I can help with this :-)

derickr · 2017-07-09T13:46:53Z

xdebug_gc_stats.c

+   | to obtain it through the world-wide-web, please send a note to		  |
+   | xdebug@derickrethans.nl so we can mail you a copy immediately.		  |
+   +----------------------------------------------------------------------+
+   | Authors: Derick Rethans <derick@xdebug.org>						  |


I didn't write this :-)

derickr · 2017-07-09T13:47:09Z

xdebug_gc_stats.c

+void xdebug_gc_stats_show_report()
+{
+	zend_string *key;
+	ulong num_key;


The Travis build fails on this: https://travis-ci.org/xdebug/xdebug/jobs/251438306#L281

derickr · 2017-07-09T14:10:30Z

php_xdebug.h

+	/* garbage stats */
+	zval gc_runs;
+	int gc_stats_enabled;
+	int gc_show_report;


these ints should probably be zend_bools - like other INI settings, that are a bool?

derickr · 2017-07-09T14:10:45Z

php_xdebug.h

@@ -117,6 +117,9 @@ PHP_FUNCTION(xdebug_get_profiler_filename);
 PHP_FUNCTION(xdebug_dump_aggr_profiling_data);
 PHP_FUNCTION(xdebug_clear_aggr_profiling_data);

+/* gc stats functions */
+PHP_FUNCTION(xdebug_gc_stats);


I prefer xdebug_get_gc_stats()

derickr · 2017-07-09T14:11:15Z

tests/xdebug_gc_stats1.phpt

@@ -0,0 +1,14 @@
+--TEST--
+xdebug: No memleak, return empty runs


The "xdebug: " prefix is not necessary. I would add "GC Stats: " though instead.

derickr · 2017-07-09T14:12:49Z

tests/xdebug_gc_stats1.phpt

@@ -0,0 +1,14 @@
+--TEST--
+xdebug: No memleak, return empty runs
+--FILE--


Do the tests work if zend.enable_gc is set to 0? If not, either add an --INI-- section to turn them on, or add a --SKIPIF-- (I prefer the --INI group)

derickr · 2017-07-09T14:13:39Z

xdebug.c

+
+	/* GC Stats support */
+	STD_PHP_INI_BOOLEAN("xdebug.gc_stats_enable",       "0",            PHP_INI_ALL,    OnUpdateBool,   gc_stats_enabled,       zend_xdebug_globals, xdebug_globals)
+	STD_PHP_INI_BOOLEAN("xdebug.gc_show_report",       "0",             PHP_INI_ALL,    OnUpdateBool,   gc_show_report,       zend_xdebug_globals, xdebug_globals)


Please align all the fields (with spaces).

derickr · 2017-07-09T14:14:57Z

xdebug.c

@@ -1191,6 +1203,9 @@ PHP_RINIT_FUNCTION(xdebug)
 	XG(profiler_enabled) = 0;
 	XG(breakpoints_allowed) = 1;

+	/* Initialize gc statistics */
+	array_init(&XG(gc_runs));


For almost every internal hash function, I use either xdebug_hash, or if possible, xdebug_list. As you can see in line 1207 (old) or 1222 (new) below.

I tried to change this, but this hash is not internal, as its being returned from xdebug_get_gc_stats and used in a foreach loop for printing. I can try porting all this, but from my first understanding of xdebug_hash this would require a larger rewrite and a copy from xdebug_hash to HashTable in the return function.

xdebug_llist actually seems more suited to get this working, I will try again :-)

derickr · 2017-07-09T14:16:02Z

xdebug_gc_stats.c

@@ -0,0 +1,157 @@
+/*
+   +----------------------------------------------------------------------+
+   | Xdebug																  |


Something seems very much not aligned here? But that could be GitHub rendering it stupidly.

The code needs to use spaces instead of tabs.

derickr · 2017-07-09T14:16:37Z

xdebug_gc_stats.h

+   | to obtain it through the world-wide-web, please send a note to       |
+   | xdebug@derickrethans.nl so we can mail you a copy immediately.       |
+   +----------------------------------------------------------------------+
+   | Authors: Derick Rethans <derick@xdebug.org>                          |


I didn't write this either.

derickr · 2017-07-16T09:25:44Z

Did you manage to address all the issues? I see Travis is still failing with a seg fault: https://travis-ci.org/xdebug/xdebug/jobs/252145626#L899 (Travis uses ZTS).

beberlei · 2017-09-16T19:39:43Z

@derickr just wanted to work on the output dir / name functionality and realized there is a problem with offering both a function and writing to a file. I have the xdebug.gc_stats_enable=1, then it starts collecting the information. Since xdebug.gc_output_dir would point to XDEBUG_TEMP_DIR, does that mean we always write to the file? Because what if the user wants to access only via the functions?

I could make xdebug.gc_stats_enable=.. an enum with values 0 => none, 1 => collect => 2 safe to file. Its a bit cryptic but would support both use cases.

derickr · 2017-10-02T09:40:05Z

In other cases, for example traces, I have functions: xdebug_start_trace/stop_trace; and settings: xdebug.trace_output_dir, and xdebug.auto_trace.

Only if xdebug.auto_trace is enabled, does it always write to file.

For profiling, you can only enable it through xdebug.profiler_enable (with triggers), as it needs to start with the first function call (which is {main}).

There are also specific env var/cookie triggers configured through https://xdebug.org/docs/execution_trace#trace_enable_trigger and https://xdebug.org/docs/execution_trace#trace_enable_trigger_value; and https://xdebug.org/docs/profiler#profiler_enable_trigger and https://xdebug.org/docs/profiler#profiler_enable_trigger_value.

I think I would prefer if this works exactly as the trace functionality or the profiler functionality. I don't think we should just echo a report at the end of each request.

derickr · 2017-11-18T14:04:56Z

Poke? :-)

beberlei · 2017-12-05T16:13:05Z

@derickr pong :-) this is still in the back of my mind, will get back to it :-)

derickr · 2017-12-31T10:34:39Z

xdebug_gc_stats.c

+		xdebug_gc_stats_stop();
+	} else {
+		RETVAL_FALSE;
+		php_error(E_NOTICE, "Function trace was not started");


"Function trace"? You mean "Garbage Collection Statistics" :-) (Check whether you phrase the term the same way as when "GCS was already started".

derickr · 2017-12-31T10:35:47Z

xdebug.c

@@ -164,6 +164,9 @@ ZEND_BEGIN_ARG_INFO_EX(xdebug_start_gcstats_args, ZEND_SEND_BY_VAL, ZEND_RETURN_
 	ZEND_ARG_INFO(0, fname)
 ZEND_END_ARG_INFO()

+ZEND_BEGIN_ARG_INFO_EX(xdebug_stop_gcstats_args, ZEND_SEND_BY_VAL, ZEND_RETURN_VALUE, 0)
+ZEND_END_ARG_INFO()


please use xdebug_void_args, which means it's not needed to create an empty ARG INFO for many functions.

derickr · 2017-12-31T10:36:22Z

xdebug_gc_stats.c

+}
+
+/* {{{ proto void xdebug_get_gc_total_collected_roots()
+   Return number of times garbage collection was triggered. */


Proto is wrong here. It's not the same as the previous function :-)

Configuration is done through INI settings: xdebug.gc_stats_enable=1 Enables the collection of Garbage collection stats Or explicitly in PHP code with: <?php xdebug_start_gcstats(); The output is written to a file baed on INI settings `xdebug.gc_stats_output_dir` and `xdebug.gc_stats_output_name` or explicitly as filename given to `xdebug_start_gcstats($filename)`. Collected | Efficiency% | Duration | Memory Before | Memory After | Reduction% | Function ----------+-------------+----------+---------------+--------------+------------+--------- 10000 | 100.00 % | 0.00 ms | 5539880 | 579880 | 10.47 % | bar 10000 | 100.00 % | 0.00 ms | 5540040 | 580040 | 10.47 % | Garbage::produce 4001 | 40.01 % | 0.00 ms | 2563048 | 578968 | 22.59 % | gc_collect_cycles

…trics. Number of garbage collection runs and total of collected roots metrics exist in Zend Engine, but are not exposed to userland. These are helpful to integrate in development to find out scripts that trigger GC.

derickr · 2018-01-02T10:02:40Z

I'm closing this PR now it has been merged through other ways. Thanks again!

derickr requested changes Jul 9, 2017

View reviewed changes

derickr requested changes Dec 31, 2017

View reviewed changes

beberlei force-pushed the GarbageStats branch from e01c208 to 2206d30 Compare December 31, 2017 11:04

beberlei force-pushed the GarbageStats branch from 2206d30 to 83191b7 Compare December 31, 2017 11:08

derickr added a commit that referenced this pull request Dec 31, 2017

Merged pull request #360

adefecb

derickr closed this Jan 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port garbage_stats extension into a new xdebug feature. #360

Port garbage_stats extension into a new xdebug feature. #360

beberlei commented Jul 8, 2017 •

edited

Loading

derickr commented Jul 9, 2017

derickr Jul 9, 2017

derickr Jul 9, 2017

derickr Jul 9, 2017

derickr Jul 9, 2017

derickr Jul 9, 2017

derickr Jul 9, 2017

derickr Jul 9, 2017

derickr Jul 9, 2017

beberlei Jul 10, 2017

beberlei Jul 10, 2017

derickr Jul 9, 2017

kenorb Aug 20, 2017

derickr Jul 9, 2017

derickr commented Jul 16, 2017

beberlei commented Sep 16, 2017

derickr commented Oct 2, 2017

derickr commented Nov 18, 2017

beberlei commented Dec 5, 2017

derickr Dec 31, 2017

derickr Dec 31, 2017

derickr Dec 31, 2017

derickr commented Jan 2, 2018

		@@ -0,0 +1,14 @@
		--TEST--
		xdebug: No memleak, return empty runs

Port garbage_stats extension into a new xdebug feature. #360

Port garbage_stats extension into a new xdebug feature. #360

Conversation

beberlei commented Jul 8, 2017 • edited Loading

derickr commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derickr commented Jul 16, 2017

beberlei commented Sep 16, 2017

derickr commented Oct 2, 2017

derickr commented Nov 18, 2017

beberlei commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derickr commented Jan 2, 2018

beberlei commented Jul 8, 2017 •

edited

Loading