From: Kirk, B. (JSC-EG311) <ben...@na...> - 2013-04-02 21:18:04
|
On Apr 2, 2013, at 4:12 PM, "Kirk, Benjamin (JSC-EG311)" <ben...@na...> wrote: > RemoteElem should be being created from LibMeshInit (see the Singleton::create() call and related bits in remote_elem.C) but apparently it isn't? To be more clear, Singleton::setup() should call the preregistered RemoteElemSetup::setup() function, which should create the remote_elem. All that is in remote_elem.C - maybe put a trace in the RemoteElem::create() method and see what gives?? -Ben |
From: Cody P. <cod...@gm...> - 2013-04-02 21:22:24
|
Trying this now... Cody On Tue, Apr 2, 2013 at 3:17 PM, Kirk, Benjamin (JSC-EG311) < ben...@na...> wrote: > On Apr 2, 2013, at 4:12 PM, "Kirk, Benjamin (JSC-EG311)" < > ben...@na...> wrote: > > > RemoteElem should be being created from LibMeshInit (see the > Singleton::create() call and related bits in remote_elem.C) but apparently > it isn't? > > > To be more clear, Singleton::setup() should call the preregistered > RemoteElemSetup::setup() function, which should create the remote_elem. > All that is in remote_elem.C - maybe put a trace in the > RemoteElem::create() method and see what gives?? > > -Ben > > > |
From: Cody P. <cod...@gm...> - 2013-04-02 22:22:34
|
On Tue, Apr 2, 2013 at 3:22 PM, Cody Permann <cod...@gm...> wrote: > Trying this now... > Cody > > > > On Tue, Apr 2, 2013 at 3:17 PM, Kirk, Benjamin (JSC-EG311) < > ben...@na...> wrote: > >> On Apr 2, 2013, at 4:12 PM, "Kirk, Benjamin (JSC-EG311)" < >> ben...@na...> wrote: >> >> > RemoteElem should be being created from LibMeshInit (see the >> Singleton::create() call and related bits in remote_elem.C) but apparently >> it isn't? >> >> Yeah - bad news... RemoteElem::create() is _not_ being called on several of our systems. Of course LibMeshInit is, but for some reason it's not getting all the way down into those new functions. On this particular system I'm using GCC 4.5.1 which is a terrible compiler, but that's one data point. The system that I have here where it is working is GCC 4.6.3. Our build boxes are 4.7.2 and they are also failing so.... hard to draw conclusions other than something isn't working quite right. ;) Cody > >> To be more clear, Singleton::setup() should call the preregistered >> RemoteElemSetup::setup() function, which should create the remote_elem. >> All that is in remote_elem.C - maybe put a trace in the >> RemoteElem::create() method and see what gives?? >> >> -Ben >> >> >> > |
From: Kirk, B. (JSC-EG311) <ben...@na...> - 2013-04-02 22:42:22
|
On Apr 2, 2013, at 5:22 PM, Cody Permann <cod...@gm...> wrote: > > > RemoteElem should be being created from LibMeshInit (see the Singleton::create() call and related bits in remote_elem.C) but apparently it isn't? > > > Yeah - bad news... RemoteElem::create() is _not_ being called on several of our systems. Of course LibMeshInit is, but for some > reason it's not getting all the way down into those new functions. > > On this particular system I'm using GCC 4.5.1 which is a terrible compiler, but that's one data point. The system that I have here where it is working is GCC 4.6.3. Our build boxes are 4.7.2 and they are also failing so.... hard to draw conclusions other than something isn't working quite right. ;) Argh - the easy fix is just put a naked RemoteElem::create(); call inside libmesh.C right after Singleton::setup(); problem is, this is the same mechanism used for Singleton::cleanup(), so that may fail to delete the remote_elem. But start there - it should run but may complain of leaked memory if the cleanup() mechanism is misbehaving?? Weird - *all* my compilers on linux worked fine - gcc 4.4, 4.5, 4.6, 4.7, icc 12.0, 12.1, 13.0, pgi's, open solaris… -Ben |
From: Cody P. <cod...@gm...> - 2013-04-02 22:48:05
|
On Tue, Apr 2, 2013 at 4:42 PM, Kirk, Benjamin (JSC-EG311) < ben...@na...> wrote: > On Apr 2, 2013, at 5:22 PM, Cody Permann <cod...@gm...> wrote: > > > > > RemoteElem should be being created from LibMeshInit (see the > Singleton::create() call and related bits in remote_elem.C) but apparently > it isn't? > > > > > > Yeah - bad news... RemoteElem::create() is _not_ being called on several > of our systems. Of course LibMeshInit is, but for some > > reason it's not getting all the way down into those new functions. > > > > On this particular system I'm using GCC 4.5.1 which is a terrible > compiler, but that's one data point. The system that I have here where it > is working is GCC 4.6.3. Our build boxes are 4.7.2 and they are also > failing so.... hard to draw conclusions other than something isn't working > quite right. ;) > > > Argh - > > the easy fix is just put a naked > > RemoteElem::create(); > > call inside libmesh.C right after Singleton::setup(); > > problem is, this is the same mechanism used for Singleton::cleanup(), so > that may fail to delete the remote_elem. > > But start there - it should run but may complain of leaked memory if the > cleanup() mechanism is misbehaving?? > > Weird - *all* my compilers on linux worked fine - gcc 4.4, 4.5, 4.6, 4.7, > icc 12.0, 12.1, 13.0, pgi's, open solaris… > > Alright, I'll give it a shot, I can't repeat the bad behavior on our Macs, even with the identical 4.7.2 compiler so it is a complicated problem indeed. > -Ben |
From: Roy S. <roy...@ic...> - 2013-04-03 06:41:38
|
On Tue, 2 Apr 2013, Cody Permann wrote: > > Weird - *all* my compilers on linux worked fine - gcc 4.4, 4.5, 4.6, 4.7, icc 12.0, 12.1, > > 13.0, pgi's, open solaris… > > Alright, I'll give it a shot, I can't repeat the bad behavior on our Macs, even with the > identical 4.7.2 compiler so it is a complicated problem indeed. We're now seeing remote_elem based failures on buildbot too; I'll see if I can reproduce it elsewhere. --- Roy |
From: Kirk, B. (JSC-EG311) <ben...@na...> - 2013-04-03 11:32:20
|
On Apr 3, 2013, at 1:41 AM, "Roy Stogner" <roy...@ic...> wrote: > > > On Tue, 2 Apr 2013, Cody Permann wrote: > >>> Weird - *all* my compilers on linux worked fine - gcc 4.4, 4.5, 4.6, 4.7, icc 12.0, 12.1, >>> 13.0, pgi's, open solaris… >> >> Alright, I'll give it a shot, I can't repeat the bad behavior on our Macs, even with the >> identical 4.7.2 compiler so it is a complicated problem indeed. > > We're now seeing remote_elem based failures on buildbot too; I'll see > if I can reproduce it elsewhere. Thanks. Ill prepare a pull request when I get in that undoes they change, but hopefully we can find a way to make it work! -Ben |
From: Derek G. <fri...@gm...> - 2013-04-03 16:28:25
|
Ok - we just looked into this - Ben: It looks like you're not ever creating RemoteElem which is why it's not getting registered. At the bottom of remote_elem.h you have this: extern const RemoteElem* remote_elem; But that's just creating a pointer. In the .C you have this: const RemoteElem* remote_elem; But, again, that's not creating a RemoteElem. In fact, nothing is ever creating a RemoteElem... so it doesn't get registered with the Singleton class. In remote_elem.C you could do this: extern const RemoteElem* remote_elem = new RemoteElem; And that would fix it. Derek On Wed, Apr 3, 2013 at 5:32 AM, Kirk, Benjamin (JSC-EG311) < ben...@na...> wrote: > On Apr 3, 2013, at 1:41 AM, "Roy Stogner" <roy...@ic...> > wrote: > > > > > > > On Tue, 2 Apr 2013, Cody Permann wrote: > > > >>> Weird - *all* my compilers on linux worked fine - gcc 4.4, 4.5, 4.6, > 4.7, icc 12.0, 12.1, > >>> 13.0, pgi's, open solaris… > >> > >> Alright, I'll give it a shot, I can't repeat the bad behavior on our > Macs, even with the > >> identical 4.7.2 compiler so it is a complicated problem indeed. > > > > We're now seeing remote_elem based failures on buildbot too; I'll see > > if I can reproduce it elsewhere. > > Thanks. Ill prepare a pull request when I get in that undoes they change, > but hopefully we can find a way to make it work! > > -Ben > > > ------------------------------------------------------------------------------ > Minimize network downtime and maximize team effectiveness. > Reduce network management and security costs.Learn how to hire > the most talented Cisco Certified professionals. Visit the > Employer Resources Portal > http://www.cisco.com/web/learning/employer_resources/index.html > _______________________________________________ > Libmesh-devel mailing list > Lib...@li... > https://lists.sourceforge.net/lists/listinfo/libmesh-devel > |
From: Derek G. <fri...@gm...> - 2013-04-03 16:35:18
|
Just to be completely clear: What's happening here is that that remote_elem pointer is _never_ getting initialized. Even in the cases where this was "working" on some machines... it was just because that un-initialized memory for remote_elem was non-zero... so it wasn't tripping the assert. With Parallel mesh it was probably failing spectacularly the first time you tried to access that remote_elem. Derek On Wed, Apr 3, 2013 at 10:28 AM, Derek Gaston <fri...@gm...> wrote: > Ok - we just looked into this - Ben: It looks like you're not ever > creating RemoteElem which is why it's not getting registered. > > At the bottom of remote_elem.h you have this: > > extern const RemoteElem* remote_elem; > > But that's just creating a pointer. In the .C you have this: > > const RemoteElem* remote_elem; > > But, again, that's not creating a RemoteElem. > > In fact, nothing is ever creating a RemoteElem... so it doesn't get > registered with the Singleton class. In remote_elem.C you could do this: > > extern const RemoteElem* remote_elem = new RemoteElem; > > And that would fix it. > > Derek > > > > On Wed, Apr 3, 2013 at 5:32 AM, Kirk, Benjamin (JSC-EG311) < > ben...@na...> wrote: > >> On Apr 3, 2013, at 1:41 AM, "Roy Stogner" <roy...@ic...> >> wrote: >> >> > >> > >> > On Tue, 2 Apr 2013, Cody Permann wrote: >> > >> >>> Weird - *all* my compilers on linux worked fine - gcc 4.4, 4.5, 4.6, >> 4.7, icc 12.0, 12.1, >> >>> 13.0, pgi's, open solaris… >> >> >> >> Alright, I'll give it a shot, I can't repeat the bad behavior on our >> Macs, even with the >> >> identical 4.7.2 compiler so it is a complicated problem indeed. >> > >> > We're now seeing remote_elem based failures on buildbot too; I'll see >> > if I can reproduce it elsewhere. >> >> Thanks. Ill prepare a pull request when I get in that undoes they change, >> but hopefully we can find a way to make it work! >> >> -Ben >> >> >> ------------------------------------------------------------------------------ >> Minimize network downtime and maximize team effectiveness. >> Reduce network management and security costs.Learn how to hire >> the most talented Cisco Certified professionals. Visit the >> Employer Resources Portal >> http://www.cisco.com/web/learning/employer_resources/index.html >> _______________________________________________ >> Libmesh-devel mailing list >> Lib...@li... >> https://lists.sourceforge.net/lists/listinfo/libmesh-devel >> > > |
From: Roy S. <roy...@ic...> - 2013-04-03 16:43:18
|
Derek: look more carefully. In remote_elem.C, remote_elem_setup is created. Its constructor should register RemoteElem::create() with the Singleton class. Ben: You've got a static initialization order fiasco here, I think. The remote_elem_setup gets constructed, it appends an entry to the setup_cache... *then* the setup_cache gets constructed, making it a new empty vector. --- Roy |
From: Kirk, B. (JSC-EG311) <ben...@na...> - 2013-04-03 16:43:37
|
On Apr 3, 2013, at 11:28 AM, Derek Gaston <fri...@gm...> wrote: > Ok - we just looked into this - Ben: It looks like you're not ever creating RemoteElem which is why it's not getting registered. > > At the bottom of remote_elem.h you have this: > > extern const RemoteElem* remote_elem; > > But that's just creating a pointer. In the .C you have this: > > const RemoteElem* remote_elem; > > But, again, that's not creating a RemoteElem. > > In fact, nothing is ever creating a RemoteElem... so it doesn't get registered with the Singleton class. In remote_elem.C you could do this: > > extern const RemoteElem* remote_elem = new RemoteElem; That would be at risk of creating the RemoteElem before its underlying reference counter is set up, which is what got us here in the first place... in remote_elem.C, I am doing this: namespace { // Class to be dispatched by Singleton::setup() // to create the \p RemoteElem singleton. // While this actual object has file-level static // scope and will be initialized before main(), // importantly the setup() method will not be invoked // until after main(). class RemoteElemSetup : public Singleton::Setup { void setup () { RemoteElem::create(); } } remote_elem_setup; } the remote_elem_setup should get instantiated statically, and its *constructor* adds (this) tot the list of objects to be invoked by Singleton::setup(). In turn, that should call remote_elem_setup.setup() indirectly, and create the singleton. This is precisely the mechanism we described for singleton desctructors, now turned on its head. The only problem I can see is if the remote_elem_setup object is getting created before the container inside libmesh_singleton.C, resulting in undefined behavior. -Ben |
From: Derek G. <fri...@gm...> - 2013-04-03 16:47:43
|
I can't see how the container is not created yet... that would be a segfault when you try to push_back on it.... not too mention that the code that actually does the pushing back is in Singleton.C which is where the container is... so it would have had to have been created by the time you call that function... Derek On Wed, Apr 3, 2013 at 10:43 AM, Kirk, Benjamin (JSC-EG311) < ben...@na...> wrote: > On Apr 3, 2013, at 11:28 AM, Derek Gaston <fri...@gm...> wrote: > > > Ok - we just looked into this - Ben: It looks like you're not ever > creating RemoteElem which is why it's not getting registered. > > > > At the bottom of remote_elem.h you have this: > > > > extern const RemoteElem* remote_elem; > > > > But that's just creating a pointer. In the .C you have this: > > > > const RemoteElem* remote_elem; > > > > But, again, that's not creating a RemoteElem. > > > > In fact, nothing is ever creating a RemoteElem... so it doesn't get > registered with the Singleton class. In remote_elem.C you could do this: > > > > extern const RemoteElem* remote_elem = new RemoteElem; > > > That would be at risk of creating the RemoteElem before its underlying > reference counter is set up, which is what got us here in the first place... > > in remote_elem.C, I am doing this: > > namespace > { > // Class to be dispatched by Singleton::setup() > // to create the \p RemoteElem singleton. > // While this actual object has file-level static > // scope and will be initialized before main(), > // importantly the setup() method will not be invoked > // until after main(). > class RemoteElemSetup : public Singleton::Setup > { > void setup () > { > RemoteElem::create(); > } > } remote_elem_setup; > } > > > the remote_elem_setup should get instantiated statically, and its > *constructor* adds (this) tot the list of objects to be invoked by > Singleton::setup(). In turn, that should call remote_elem_setup.setup() > indirectly, and create the singleton. > > This is precisely the mechanism we described for singleton desctructors, > now turned on its head. > > The only problem I can see is if the remote_elem_setup object is getting > created before the container inside libmesh_singleton.C, resulting in > undefined behavior. > > -Ben > > |
From: Derek G. <fri...@gm...> - 2013-04-03 16:49:17
|
But yes - I missed the creation of that Setup object... it's not often that you see this type of construction: class RemoteElemSetup : public Singleton::Setup { void setup () { RemoteElem::create(); } } remote_elem_setup; I totally missed that "remote_elem_setup" guy hanging off the end there ;-) Derek On Wed, Apr 3, 2013 at 10:47 AM, Derek Gaston <fri...@gm...> wrote: > I can't see how the container is not created yet... that would be a > segfault when you try to push_back on it.... not too mention that the code > that actually does the pushing back is in Singleton.C which is where the > container is... so it would have had to have been created by the time you > call that function... > > Derek > > > On Wed, Apr 3, 2013 at 10:43 AM, Kirk, Benjamin (JSC-EG311) < > ben...@na...> wrote: > >> On Apr 3, 2013, at 11:28 AM, Derek Gaston <fri...@gm...> wrote: >> >> > Ok - we just looked into this - Ben: It looks like you're not ever >> creating RemoteElem which is why it's not getting registered. >> > >> > At the bottom of remote_elem.h you have this: >> > >> > extern const RemoteElem* remote_elem; >> > >> > But that's just creating a pointer. In the .C you have this: >> > >> > const RemoteElem* remote_elem; >> > >> > But, again, that's not creating a RemoteElem. >> > >> > In fact, nothing is ever creating a RemoteElem... so it doesn't get >> registered with the Singleton class. In remote_elem.C you could do this: >> > >> > extern const RemoteElem* remote_elem = new RemoteElem; >> >> >> That would be at risk of creating the RemoteElem before its underlying >> reference counter is set up, which is what got us here in the first place... >> >> in remote_elem.C, I am doing this: >> >> namespace >> { >> // Class to be dispatched by Singleton::setup() >> // to create the \p RemoteElem singleton. >> // While this actual object has file-level static >> // scope and will be initialized before main(), >> // importantly the setup() method will not be invoked >> // until after main(). >> class RemoteElemSetup : public Singleton::Setup >> { >> void setup () >> { >> RemoteElem::create(); >> } >> } remote_elem_setup; >> } >> >> >> the remote_elem_setup should get instantiated statically, and its >> *constructor* adds (this) tot the list of objects to be invoked by >> Singleton::setup(). In turn, that should call remote_elem_setup.setup() >> indirectly, and create the singleton. >> >> This is precisely the mechanism we described for singleton desctructors, >> now turned on its head. >> >> The only problem I can see is if the remote_elem_setup object is getting >> created before the container inside libmesh_singleton.C, resulting in >> undefined behavior. >> >> -Ben >> >> > |
From: Roy S. <roy...@ic...> - 2013-04-03 16:53:51
|
On Wed, 3 Apr 2013, Derek Gaston wrote: > I can't see how the container is not created yet... that would be a > segfault when you try to push_back on it.... No, that would be undefined behavior when you try to push_back on it. "undefined behavior" only means a segfault when it ends up hitting memory outside the process' current address space. > not too mention that the code that actually does the pushing back is > in Singleton.C which is where the container is... so it would have > had to have been created by the time you call that function... Doesn't matter. What C++ is doing under the hood is: Construct A Construct B except in random order (which is why stuff worked fine for Ben), not alphabetical order. And if A's constructor calls a function dealing with B, there's neither compile-time nor run-time analysis done to detect that and say "whoops, let's construct B before A". --- Roy |
From: Kirk, B. (JSC-EG311) <ben...@na...> - 2013-04-03 17:02:42
|
On Apr 3, 2013, at 11:53 AM, Roy Stogner <roy...@ic...> wrote: > > On Wed, 3 Apr 2013, Derek Gaston wrote: > >> I can't see how the container is not created yet... that would be a >> segfault when you try to push_back on it.... > > No, that would be undefined behavior when you try to push_back on it. > "undefined behavior" only means a segfault when it ends up hitting > memory outside the process' current address space. > >> not too mention that the code that actually does the pushing back is >> in Singleton.C which is where the container is... so it would have >> had to have been created by the time you call that function... > > Doesn't matter. > > What C++ is doing under the hood is: > > Construct A > Construct B > > except in random order (which is why stuff worked fine for Ben), not > alphabetical order. > > And if A's constructor calls a function dealing with B, there's > neither compile-time nor run-time analysis done to detect that and say > "whoops, let's construct B before A". I'm having a hard time visualizing that, but if that is indeed the issue shouldn't it suffice to move SetupList setup_cache; from the anonymous namespace instead into a static member of the Setup::Setup() constructor? That will guarantee it is not instantiated until it is first called, and that it will be there when needed. -Ben |
From: Kirk, B. (JSC-EG311) <ben...@na...> - 2013-04-03 16:54:05
|
On Apr 3, 2013, at 11:47 AM, Derek Gaston <fri...@gm...> wrote: > I can't see how the container is not created yet... that would be a segfault when you try to push_back on it.... not too mention that the code that actually does the pushing back is in Singleton.C which is where the container is... so it would have had to have been created by the time you call that function… That was my thinking - that by putting the constructor implementation in the same translation unit as the buffer we should be fine… which leaves only the possibility that RemoteElemSetup is somehow not properly calling he Singleton::Setup base class constructor? Can you apply this on the affected systems and make sure that's not the case? diff --git a/src/base/libmesh_singleton.C b/src/base/libmesh_singleton.C index 977c743..8390db5 100644 --- a/src/base/libmesh_singleton.C +++ b/src/base/libmesh_singleton.C @@ -20,6 +20,7 @@ // Local includes #include "libmesh/libmesh_singleton.h" #include "libmesh/threads.h" +#include "libmesh/print_trace.h" // C/C++ includes #include <vector> @@ -63,6 +64,7 @@ namespace libMesh Singleton::Setup::Setup () { + print_trace(); SingletonMutex::scoped_lock lock(setup_mtx); setup_cache.push_back (this); |
From: Roy S. <roy...@ic...> - 2013-04-03 16:57:30
|
On Wed, 3 Apr 2013, Kirk, Benjamin (JSC-EG311) wrote: > which leaves only the possibility that RemoteElemSetup is somehow > not properly calling he Singleton::Setup base class constructor? > > Can you apply this on the affected systems and make sure that's not > the case? I already ran through this in the debugger. Singleton::Setup::Setup is indeed being called from the RemoteElemSetup. --- Roy |
From: Derek G. <fri...@gm...> - 2013-04-03 17:01:14
|
On Wed, Apr 3, 2013 at 10:53 AM, Roy Stogner <roy...@ic...>wrote: > Doesn't matter. > > What C++ is doing under the hood is: > > Construct A > Construct B > > except in random order (which is why stuff worked fine for Ben), not > alphabetical order. > > And if A's constructor calls a function dealing with B, there's > neither compile-time nor run-time analysis done to detect that and say > "whoops, let's construct B before A". > It's simply not possible Roy. The code to do the push_back is in the same .C file with the thing it's trying to push_back on. If you can't count on statics in the same .C file being created before you try to use them.... then we're all screwed! That static will get created when the translation unit is loaded to run Singleton::Setup::Setup() (if not before then). So it has to exist by the time you try to push_back to it. Something else is happening here. Derek |
From: Roy S. <roy...@ic...> - 2013-04-03 17:04:09
|
On Wed, 3 Apr 2013, Derek Gaston wrote: > It's simply not possible Roy. The code to do the push_back is in > the same .C file with the thing it's trying to push_back on. Right. Which doesn't matter. C++ guarantees that two static objects declared in the same file will be initialized in the order of declaration, but it absolutely does not guarantee that a static object declared in a file will be initialized before any object in any other file calls any method in the first file. > If you > can't count on statics in the same .C file being created before you > try to use them.... then we're all screwed! This is actually not too hard to fix. We just use the "Construct on First Use" idiom for the containers. I'll demonstrate with a patch shortly. > That static will get created when the translation unit is loaded to > run Singleton::Setup::Setup() (if not before then). No, it really really won't. There is no "interrupt one constructor to run another just-in-time" code in C++. --- Roy |
From: Roy S. <roy...@ic...> - 2013-04-03 17:10:17
Attachments:
construct_on_first_use.diff
|
On Wed, 3 Apr 2013, Roy Stogner wrote: > This is actually not too hard to fix. We just use the "Construct on > First Use" idiom for the containers. I'll demonstrate with a patch > shortly. Try the attached. It fixes the problem for me, but since static initialization order problems are pseudorandom it's possible that there's some other bug that I'm just not managing to trigger or not managing to trigger anymore. --- Roy |
From: Roy S. <roy...@ic...> - 2013-04-03 17:31:25
|
On Wed, 3 Apr 2013, Derek Gaston wrote: > If you can't count on statics in the same .C file being created > before you try to use them.... then we're all screwed! Two random semi-related thoughts: Somehow I read that sentence in Mark Hamill's rising-in-despair voice from the end of Empire, and I really wanted to respond with "Search your feelings, you *know* it to be true!" I have apparently internalized the C++ philosophy to such an extent that, when two design alternatives are "always do a safe-but-slower thing" versus "do the fastest-but-dangerous thing by default but provide a more verbose way to do the safe-but-slower thing", I neither choose nor expect anyone else to choose the former option. This might not be good. Cody etc., let me know if that patch file fixes things for everyone else? If so then I'll prepare a pull request reverting Ben's debugging stuff and adding a slightly-more-efficient version of my patch. --- Roy |
From: Kirk, B. (JSC-EG311) <ben...@na...> - 2013-04-03 17:36:04
|
On Apr 3, 2013, at 12:31 PM, Roy Stogner <roy...@ic...> wrote: > Cody etc., let me know if that patch file fixes things for everyone > else? If so then I'll prepare a pull request reverting Ben's > debugging stuff and adding a slightly-more-efficient version of my > patch. Thanks! |