Everything

php

Re: ZTS issues

php.internals

Michael Sisolak

22 years ago

> I have been looking at the IIS isapi issues and came up with the
> following patch:
>
> http://www.ctindustries.net/patches/zts-php5.diff.txt

Rob,

I've done some work in the past on ISAPI threading issues (although I
by no means claim to understand how all the ZTS stuff works).  Can you
explain why you added the calls to ts_free_thread() after the
DLL_PROCESS_ATTACH and then each call to HttpExtensionProc()?  In both
cases the actual thread under IIS does continue to exist and can be
called on again when a new HTTP request arrives at the server.  It
would appear to me that this change would be causes PHP to have to
reinitialize with each request.  Is there a specific issue that you
were able to fix with this change?

Michael Sisolak
msisolak@yahoo.com


__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree

Andi Gutmans

22 years ago

At 08:06 PM 11/17/2003 -0800, Michael Sisolak wrote:

> > I have been looking at the IIS isapi issues and came up with the
> > following patch:
> >
> > http://www.ctindustries.net/patches/zts-php5.diff.txt
>
>Rob,
>
>I've done some work in the past on ISAPI threading issues (although I
>by no means claim to understand how all the ZTS stuff works).  Can you
>explain why you added the calls to ts_free_thread() after the
>DLL_PROCESS_ATTACH and then each call to HttpExtensionProc()?  In both
>cases the actual thread under IIS does continue to exist and can be
>called on again when a new HTTP request arrives at the server.  It
>would appear to me that this change would be causes PHP to have to
>reinitialize with each request.  Is there a specific issue that you
>were able to fix with this change?

Rob,

Also, how sure are you about this patch? What tool did you use for
debugging. The code changes you made change some of the most fragile code
in PHP so I want to be absolutely sure it's OK. I'm going to run it by Zeev
who knows this part of the code the best.

Andi

Zeev Suraski

22 years ago

At 06:06 18/11/2003, Michael Sisolak wrote:

> > I have been looking at the IIS isapi issues and came up with the
> > following patch:
> >
> > http://www.ctindustries.net/patches/zts-php5.diff.txt
>
>Rob,
>
>I've done some work in the past on ISAPI threading issues (although I
>by no means claim to understand how all the ZTS stuff works).  Can you
>explain why you added the calls to ts_free_thread() after the
>DLL_PROCESS_ATTACH and then each call to HttpExtensionProc()?  In both
>cases the actual thread under IIS does continue to exist and can be
>called on again when a new HTTP request arrives at the server.  It
>would appear to me that this change would be causes PHP to have to
>reinitialize with each request.  Is there a specific issue that you
>were able to fix with this change?

Same questions here...  The extra calls to ts_free_thread() seem
bogus.  Not sure yet about the other changes although they probably make
more sense - I never made too much effort to ensure that the cleanup of a
thread is very complete...

Zeev

Rob Richards

22 years ago

From: Zeev Suraski

> Same questions here...  The extra calls to ts_free_thread() seem
> bogus.  Not sure yet about the other changes although they probably make
> more sense - I never made too much effort to ensure that the cleanup of a
> thread is very complete...

You are correct. The ts_free_thread calls arent needed. Finally found out
that there was a Layered Service Provider running which was screwing
everything up as it kept attaching to every iis request and hosed things.
My system kept allocating memory until finally crashing. After removing the
LSP, iis started working much better as running apache bench with 20
simultaneous users each running 500 requests, memory only increased about
10MB which is pretty much correct concerning the amount of data gets stored
on each thread once its initialized. Good news however is that some
commercial software which wasnt working and the problems blamed on
hyperthreading now work fine with the LSP removed.

The other changes however are valid as the threads were not fully cleaned up
and left large chunks of memory floating around after shutdown.

From: Andi Gutmans

> Also, how sure are you about this patch? What tool did you use for
> debugging.

To check the memory, I had _CrtDumpMemoryLeaks() log to a file after the
tsrm_shutdown to test final leaks. Had to step through the code otherwise
for other parts as the system would bail out without any indication of
where. For the patch, the code for the isapi and apache2 sapi can be dumped
as those added the ts_free_thread() calls, which I see are wrong. As I
mentioned, the other changes work fine in windows. If no one sees anything
glaring in those changes, I will gladly test them in another environment.

Rob

Michael Sisolak

22 years ago

--- Rob Richards <rrichards@ctindustries.net> wrote:

> To check the memory, I had _CrtDumpMemoryLeaks() log to a file after
> the tsrm_shutdown to test final leaks. Had to step through the code
> otherwise for other parts as the system would bail out without any
> indication of where. For the patch, the code for the isapi and
> apache2 sapi can be dumped as those added the ts_free_thread() calls,

> which I see are wrong. As I mentioned, the other changes work fine in

> windows. If no one sees anything glaring in those changes, I will
> gladly test them in another environment.

Rob,

It looks like most of these changes are PHP5 specific (either the code
changes like many of the free()s were already in PHP4, or involve
things like the global constants cache which don't exist there) which I
haven't looked at enough to comment on.  The only question I have is
about the freeing of the BG(url_adapt_state_ex) tags hash.  I see you
added the code to basic_globals_dtor to free it after each request
(which makes sense as it's wiped in basic_globals_ctor), but why only
for the ZTS build?  Is there a reason that this couldn't be used for
all the builds (and remove the similar code for non-ZTS-only builds
from the url_scanner shutdown)?  Hummm... I also just saw the
php_url_scanner_ex_deactivate() function.  Maybe the hash free should
be there instead?

Michael

__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree

Rob Richards

22 years ago

From: Michael Sisolak

> It looks like most of these changes are PHP5 specific (either the code
> changes like many of the free()s were already in PHP4, or involve
> things like the global constants cache which don't exist there) which I
> haven't looked at enough to comment on.  The only question I have is
> about the freeing of the BG(url_adapt_state_ex) tags hash.  I see you
> added the code to basic_globals_dtor to free it after each request
> (which makes sense as it's wiped in basic_globals_ctor), but why only
> for the ZTS build?  Is there a reason that this couldn't be used for
> all the builds (and remove the similar code for non-ZTS-only builds
> from the url_scanner shutdown)?  Hummm... I also just saw the
> php_url_scanner_ex_deactivate() function.  Maybe the hash free should
> be there instead?

Looking at how its laid out, the php_url_scanner_ex_deactivate() is called
on request shutdown. The basic_globals_dtor is only called when the thread
is cleaned up (either an explicit call to ts_free_thread or tsrm_shutdown).
In a ZTS build, the freeing of the BG(url_adapt_state_ex) tags hash doesnt
need to happen until the thread is being killed as PHP_INI_MH(OnUpdateTags),
called at the start of a request, in url_scanner_ex.c will destroy the
hashtable if it has already been malloc'd otherwise it will malloc it. If
the hash free were moved to php_url_scanner_ex_deactivate(), then it would
always need to be malloc'd on each new request. Though this is an option, it
seems the point of ZTS was to reduce the amount of malloc/frees needed and
reuse memory (pretty much why I wasnt supposed to add the ts_free_thread
calls at the end of the requests). For a non-ZTS build, the hash is
destroyed in the PHP_MSHUTDOWN_FUNCTION(url_scanner) and as it should be
running on the same thread for the requests, this should be fine.

It possibly could be moved to the basic_globals_dtor function as that is
called by non-ZTS builds as well in PHP_MSHUTDOWN_FUNCTION(basic). Then it
would all be in a common place.

Rob

Michael Sisolak

22 years ago

--- Rob Richards <rrichards@ctindustries.net> wrote:

> If the hash free were moved to php_url_scanner_ex_deactivate(), then
> it would always need to be malloc'd on each new request. Though this
> is an option, it seems the point of ZTS was to reduce the amount of
> malloc/frees needed and reuse memory (pretty much why I wasnt
> supposed to add the ts_free_thread calls at the end of the requests).
> For a non-ZTS build, the hash is destroyed in the
> PHP_MSHUTDOWN_FUNCTION(url_scanner) and as it should be
> running on the same thread for the requests, this should be fine.
>
> It possibly could be moved to the basic_globals_dtor function as that
> is called by non-ZTS builds as well in PHP_MSHUTDOWN_FUNCTION(basic).
> Then it would all be in a common place.

Rob,

That explination makes a lot of sense.  I'm +1 on having a ZTS/non-ZTS
common free for BG(url_adapt_state_ex).tags in basic_globals_dtor().

Michael

__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree

Rob Richards

22 years ago

From: Michael Sisolak

> That explination makes a lot of sense.  I'm +1 on having a ZTS/non-ZTS
> common free for BG(url_adapt_state_ex).tags in basic_globals_dtor().

Here's the updated patch for that stuff
http://www.ctindustries.net/patches/zts-php5-20031120.diff.txt
Also fixes an issue last patch introduced in non-ZTS builds.
zend_register_internal_class() was also added to
zend_register_iterator_wrapper(). If for some reason this shouldnt be
registered as an internal class, then zend_iterator_class_entry is going to
need to be cleaned up at the end of processing.

Rob