Hello Moritz, ok the ini option makes sense and you changed the name. Also the license is of course ok, infact aiming for inclusion into the core at some time which might make sense for the sapi stuff requires php license. However now to the non fun part. We are kind of picky about coding standards: http://cvs.php.net/viewcvs.cgi/*checkout*/php-src/CODING_STANDARDS
for a start: - move all variable declarations to the top of their respective block - change all c++ comments to c comments - You have this in your code: #ifdef ZTS #error I dont like ZTS #endif but also seem to be using TSRM macros correct, so what is true in case ZTS is not supported you should add checks in your configure.m4 - white space, white space, white space erm did i say white space? - you have your own ASSERT but don't protect it from non debug builds - your time_get() implementation is pretty system dependent, well this is no longer for a start and can be fixed by people with other systems if they find your stuff interesting Do you have any cool usage of miniphp you could tell about in short? best regards marcus Sunday, May 21, 2006, 12:25:58 AM, you wrote: > Hi, > First thanks a lot for your quick review ;) > I subscribed to the pecl-dev list, but have not yet got any response, maybe > something is temporarily broken there? > So, now about the profiling extension: > License would be the php license, or anything that suits ;) > The INI-Options have a reason. The profiler should report compile times for > the PHP files, and at the point where profiler_start() is called, the files > are already compiled. > One solution would be to always profile, which I don't want because it is a > (small) performance impact, even if you do not use the profiler. > Another solution, which I'm going to implement, is to always profile the > first call to zend_compile_file, and search the resulting zend_op_array for > a call to profiler_quickdump. If it's found, profiling is turned on. > The popup is optional, reason for it is that some pages are designed in a > way that additional data at the bottom of the page is either not readable or > is displayed in strange optics caused by stylesheets. I think a popup is a > nice solution for this, if people need it. > I changed the name to "profiler", phprofiler was really hard to type ;) > I fixed the segfault, and will go through the source again checking it for > security. > Moritz > -----Original Message----- > From: Antony Dovgal [mailto:[EMAIL PROTECTED] > Sent: Saturday, May 20, 2006 11:56 PM > To: Moritz Möller > Cc: internals@lists.php.net; [EMAIL PROTECTED] > Subject: Re: [PHP-DEV] profiling extension and minimal-php-sapi > You need to contact [EMAIL PROTECTED] to apply for PECL account. > I CC'ed this list. > On 21.05.2006 01:16, Moritz Möller wrote: >> Hello, >> I made two extensions for PHP that I?m want to share, but I do not really >> know how to do it ;) > Basically you've already done (almost) everything. > You also need to clarify which license you're going to use for those > extensions > (from what I can see it's PHP License and it's fine). >> One is a profiling extension, called PhProfile: >> >> - works with zend optimizer and similar (as opposed to xdebug) >> >> - counts the number of function calls and the total send self > time >> for each function >> >> - usable either by calling phprofile_start(); at the beginning > and >> print_r(phprofile_get()) at the end of the script or >> >> - by setting up a ?dump-profile? parameter in your php.ini (like >> phprofile.quick_dump_key=?prof?) and appending the parameter prof to the > url >> (script.php?prof=1). If done, a html table with all function calls sorted > by >> self time is appended to the page. >> >> - If phprofile.quick_dump_mode is set to ?2?, the profiling data > is >> displayed in a popup > Personally, I like the idea of this one. > But there are several things I'd like to change: > 1) I don't see any sense in these INI options. > I think it's pretty easy to add if (isset($_REQUEST['profiler_or_anything > you like'])) profiler_start(); > And having an extension that uses popups (which every browser blocks > nowadays) is not really good idea too IMO. > Both Javascript and HTML do not look really good in console.. > 2) I don't like the name =). > IMO "profiler" is enough. This applies to functions too, of course. > I didn't look into the implementation too close, but there are several > problems I've spotted: > 1) Did you try to run phprofiler_get() before phprofiler_start()? > You should get a segfault, because PHPROFILER_G(data) is not initialized at > this point. > Btw, there is profiler_start(), but no profiler_end/stop()? > 2) You need to make the code to comply with PECL coding standards if you > want it to be accepted. > Windows EOLs, one-line IF's are no good.. > 3) This code won't compile with MSVC and gcc 2.9x: > static void phprofiler_zend_execute_internal(zend_execute_data* > execute_data_ptr, int return_value_used TSRMLS_DC) > { > if (!PHPROFILER_G(enabled)) { > phprofiler_saved_zend_execute_internal(execute_data_ptr, > return_value_used TSRMLS_CC); > return; > } > const char* function_name = > execute_data_ptr->function_state.function->common.function_name; > ... > } > You have to move "const char*" declaration to the beginning to of the func. > -- > Wbr, > Antony Dovgal Best regards, Marcus -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
