Hi Siriam, Sriram Natarajan wrote: > HI > Looks good to me except for couple of suggestions > > - Did you try compiling with -xO4 ? With respect to Apache, I noticed > around 5% gain by simply compiling with apache with -xO4. It seems that > Apache 2.2.9 is going to come with this flag (-xO4). Af course, you > could integrate as is and then file a bug to track this issue. >
We are taking the cautious approach initially, our tests haven't shown any significant benefits with -xO4 over -xO3 at least not on the workloads I've tested with. As you say, we can consider other optimisations post integration. > - If I am not mistaken, there is no /usr/bin/php-cgi . php-cgi is only > under /usr/php5/5.2.4/bin/php-cgi. Once this is integrated, I will file > a bug to ensure that PHP 5.2.6 integration handles this change in > location as well. > You're right, in order to run the test suite on an installed version of Lighttpd we tend to link php-cgi into /usr/bin as opposed to changed 5 or 6 config files used by the tests. I'll change it back to 5.2.4 path, these paths will be invalid soon anyway, but at least for now they'll be based on the real world. Thanks for the review Amanda > - Sriram > > Amanda Waite wrote: > >> http://cr.opensolaris.org/~tekgrrl/lighttpd-putback-1 is an updated >> webrev based on comments received so far. >> >> Items addressed: >> >> - Indentation of Makefile.sfw >> - Changed the fcgi-php.conf file to use a variable for the php command >> string and added a note >> in the comments at the head of the file. >> - Created a sunman-stability file and modified the man page to remove >> duplicate information. >> - Changed the install script to use _install M to install the man page >> >> Items not addressed >> >> - Use of gtar -xpf >> - We didn't actually change this as it seems the most popular way of >> doing it and because I'm >> scared of breaking what works >> >> - service instance name change. >> - Needs further discussion and some kind of agreement on convention, >> is currently consistent with >> Apache 2.2 >> >> - Running _install to install binaries >> - We believe that post_process and post_process_so do the same thing >> and it seems that all >> binaries are run through one of these commands >> >> Please review and let us have your feedback. >> >> Thanks >> >> Amanda >> >> >> Amanda Waite wrote: >> >> >>> I've looked at the options for having lighttpd pick up the PHP paths >>> dynamically and there really are none. The only thing I can do is to >>> make the fcgi-php.conf file into a sample file or remove it >>> altogether. Leaving it as it is doesn't seem to be an option as the >>> path to the ini file will be invalid soon. >>> >>> I can define variables in the config file so I could set the paths to >>> be variables and then make it clear that they need to be set in order >>> to make it work correctly. >>> >>> Thanks >>> >>> Amanda >>> >>> >>> Amanda Waite wrote: >>> >>> >>>> Thanks for reviewing this. See inline comments: >>>> >>>> Sriram Natarajan wrote: >>>> >>>> >>>> >>>>> HI >>>>> >>>>> - You might want to take a look at the indentation within >>>>> lighthttpd/Makefile.sfw >>>>> >>>>> - Why not simply do it as gtar xf and avoid manually changing the >>>>> permission later >>>>> >>>>> + $(GTAR) xpf - --no-same-owner >>>>> + touch $(LIGHTTPD)/configure >>>>> + find $(LIGHTTPD) -type d -exec /usr/bin/chmod 755 "{}" \; >>>>> + find $(LIGHTTPD) -type f -exec /usr/bin/chmod ugo+r "{}" \; >>>>> >>>>> >>>>> >>>> The way we are doing it here maybe over cautious but it does seem to >>>> avoid any possible permissions problems. I'd be happy to do it the >>>> simpler way, but am thinking that albeit historical, this was done >>>> for other integrations for a reason. I wonder if anyone has any >>>> knowledge of why this was done this way, if not I'll just change it >>>> to the simpler form. >>>> >>>> >>>> >>>> >>>>> - Instead of using hard coded PHP path , you might want to use it >>>>> in a dynamic way. This is because PHP 5.2.6 integration is coming >>>>> pretty soon and is going to have a different file layout. >>>>> >>>>> >>>>> >>>> I could just use /usr/bin/php-cgi for the executable. Will 5.2.6 use >>>> the same /usr/bin/php-cgi symlink? Would that be stable enough? For >>>> the php.ini file things are a little more difficult and I need to >>>> look into this some more. >>>> >>>> >>>> >>>> >>>>> - With MySQL, you will now start the server as svcadm enable >>>>> mysql:version_50. Looks like, Apache and Lighttpd is going to start >>>>> their servers like svcadm enable http:apache22 and >>>>> http:lighthttpd14. Should we not all do this in a consistent way ? >>>>> I guess, this is a separate discussion. >>>>> >>>>> >>>>> >>>> I agree, we should have this discussion. version_50 is somewhat >>>> different from lighttpd14, it feels that 'version' and '50' need to >>>> be separated with an underscore, but I don't get the same feeling for >>>> lighttpd14 or apache22. I wonder if mysql:version_50 might have >>>> worked as mysql:50 >>>> >>>> >>>> >>>> >>>> >>>> >>>>> - within the man page, there is a reference to memcached. Probably >>>>> a miss in cut and paste . >>>>> >>>>> >>>>> >>>> Great spot, I shall fix both of these references >>>> >>>> >>>> >>>> >>>>> - While installing man pages, should we not use the convention of >>>>> _install M ? >>>>> >>>>> >>>>> >>>> I've done this with the main lighttpd.1 page, which is a page that I >>>> created, to make this work I created the sunman-stability sed script >>>> required by _install M. The Lighttpd man pages are patched to add >>>> some more detail but they are installed by the standard make, not by >>>> the script. I can changed this, but looking at PHP, MySQL and Apache, >>>> none of their community provided man pages have the sunman-stability >>>> data appended. >>>> >>>> >>>> >>>> >>>> >>>>> - If the binaries and libraries are installed using _install , then >>>>> we can be sure that the symbols get stripped out. I see some >>>>> components doing this and some not. I am not sure, what is the >>>>> convention within SFW ? >>>>> >>>>> >>>>> >>>> All 3 of the executables are stripped by the $SRC/tools/post_process >>>> command and all of the libs are stripped by >>>> $SRC/tools/post_process_so. I don't think I'm missing any and I was >>>> fairly confident that this was the right way to do it if not using >>>> _install. Did you see any specifically that I missed? >>>> >>>> >>>> Thanks >>>> >>>> Amanda >>>> >>>> >>>> >>>>> Hope this helps >>>>> Sriram >>>>> >>>>> Amanda Waite wrote: >>>>> >>>>> >>>>> >>>>>> Hi there, >>>>>> >>>>>> Before we do the putback on Lighttpd we'd like to get a few more >>>>>> eyeballs to go over the code and give us a thorough review. Not a >>>>>> great deal has changed since the last code review, but we have >>>>>> removed the horrible LD_LIBRARY_PATH statement from Makefile.sfw >>>>>> and no longer ask the configure script to use mysql_config. >>>>>> >>>>>> The code review is at >>>>>> http://cr.opensolaris.org/~tekgrrl/lighttpd-putback/ >>>>>> >>>>>> We would really appreciate your feedback. >>>>>> >>>>>> Thanks >>>>>> >>>>>> Amanda - ISV-Engineering >>>>>> _______________________________________________ >>>>>> >>>>>> >>>>>> webstack-discuss mailing list >>>>>> webstack-discuss at opensolaris.org >>>>>> http://mail.opensolaris.org/mailman/listinfo/webstack-discuss >>>>>> >>>>>> >>>>>> >>>> _______________________________________________ >>>> >>>> >>>> webstack-discuss mailing list >>>> webstack-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/webstack-discuss >>>> >>>> >>>> >>> >>> >> _______________________________________________ >> >> >> webstack-discuss mailing list >> webstack-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/webstack-discuss >> >> > _______________________________________________ > > > webstack-discuss mailing list > webstack-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/webstack-discuss >
