rfc:error-optimizations

This is an old revision of the document!


Request for Comments: Error formatting optimizations

This RFC proposes a way to optimize our errors, by performing a type of just-in-time formatting to reduce the additional costs by formatting errors thats never shown.

Introduction

Every error in PHP is formatting in a sprintf()-alike syntax using one of our many error handling functions, every time an error, warning or notice occurs, we hit the error handlers, format them even if we never wish to display the errors. This RFC proposes a way that could be used to reduce that.

Implementation

This RFC's implementation, proposes that we extends the EG() globals (executor) inside the Engine, to contain four new fields:

        ...
        zend_bool error_stack_enabled: 1;
        zend_bool error_stack_logging: 0;
        zend_error_arguments error_stack[];
        int error_stack_size: 0;
        ...

The new structure, “zend_error_arguments” looks like the following:

struct {
	/* Message format */
	const char *format;
 
        /* Error type */
        const short type;
 
	/* File, and line number, if any */
	const char *filename;
	const int lineno;
 
	/* Arguments */
	va_args arguments;
} _zend_error_arguments;
 
typedef struct _zend_error_arguments zend_error_arguments;

The error_stack_logging executor global needs to match that of INI_BOOL(“log_errors”) and is only enabled if error_stack_enabled (INI_BOOL(“display_errors”) is off) is 1, which should be handled in main/ at PHP, outside the Engine which just sets a default. When we hit an error, it will stack the error using the error arguments structure and stash it into the global. If display_errors is off and log_errors is on, then dispatch it directly to the logging buffer, but only stack the last occured one, meaning that if two errors occurs in a row, with log_errors set to on and display errors set to off, the second call will override the last argument, using the error_stack_size global:

zend_error_arguments arguments;
 
/* put the data into the arguments structure */
 
if (EG(error_stack_enabled) && EG(error_stack_logging)) {
        EG(error_stack)[EG(error_stack_size)] = arguments;
 
        if (EG(error_stack_logging)) {
                  /* dispatch to error logging hook */
        } else {
                  /* no logging, increase the stack size */
 
                  ++EG(error_stack_size);
        }
} else {
        /* BC code */
}

error_get_last()

error_get_last() should internally check if EG(error_stack_enabled) is on, like:

if (EG(error_stack_enabled) && EG(error_stack_size)) {
        char *error = (char *) emalloc(sizeof(zend_error_arguments));
        int error_length;
 
        /* dispatch to formatting function, and copy it into, the "error" variable */
        /* NOTE: This is just a pseudo function name for formatting */
        zend_format_error_arguments(&error, &error_length, EG(error_stack)[EG(error_stack_size)]);
 
        RETURN_STRINGL(error, error_length, 0, 1);
} else {
        /* BC code */
}

$php_errormsg

This one is a tricky one, as we do not have any hooks for altering variables at reading, nor do we have JIT assignments. I think the best solution here is to simply remove $php_errormsg and require userland to use error_get_last() if they *REALLY* want the last errors, without depending on the track_errors ini options to be on.

If its not removed, then we cannot gain any optimization with track_errors = On at all, so if thats the case, it has to be taken into account when initializing the error_stack executor globals.

Memory usage

Obvious the memory usage here can grow quite rapidly, but people who would use this feature already takes great care of their code to not assume the opposite.

Additional notes

  • While altering the internal logic of zend_error() and zend_error_cb() to match that of the above, it might be worth passing the TSRMLS macros to it, to save some TS performance at every call, which itself is a bonus for multithreaded SAPIs.
rfc/error-optimizations.1282663093.txt.gz · Last modified: 2017/09/22 13:28 (external edit)