libMesh: A C++ Finite Element Library

On Apr 22, 2011, at 9:02 AM, Roy Stogner wrote:

> In particular we can see failed assertions (and probably crashes or
> corruption) in multithreaded code that relys on the mesh-provided
> point_locator() incorrectly: MeshFunction, PeriodicBoundaries,
> ExactSolution/ExactErrorEstimator, and RBEIMSystem are likely
> affected.

Thank god!  We've known for a while now (like over a year) that PeriodicBoundaries didn't work with threading... but never had time to fully investigate!  This could be it!

> I'd like to change point_locator() to return an AutoPtr instead, an
> API-incompatible change.  The alternative would be to just deprecate
> the existing function and create a new one with a different name.  Any
> preferences?

Just change it.  I'm sure there aren't _too_ many people using PointLocator... and we can easily change the few places we use it.

Derek

On Fri, 22 Apr 2011, Derek Gaston wrote:

> On Apr 22, 2011, at 9:02 AM, Roy Stogner wrote:
>
>> In particular we can see failed assertions (and probably crashes or
>> corruption) in multithreaded code that relys on the mesh-provided
>> point_locator() incorrectly: MeshFunction, PeriodicBoundaries,
>> ExactSolution/ExactErrorEstimator, and RBEIMSystem are likely
>> affected.
>
> Thank god!  We've known for a while now (like over a year) that
> PeriodicBoundaries didn't work with threading... but never had time
> to fully investigate!  This could be it!

Yeah, I'm now starting to realize how dangerous it is for me to keep
adding more and more regression tests to BuildBot.  Combine that with
my Monkey's Paw wish to have people write more example codes and my
Rain Man-like obsession with cleaning up every resulting mess, and
it's making for a lot of work at a very inconvenient time.  :-P

>> I'd like to change point_locator() to return an AutoPtr instead, an
>> API-incompatible change.  The alternative would be to just deprecate
>> the existing function and create a new one with a different name.  Any
>> preferences?
>
> Just change it.  I'm sure there aren't _too_ many people using
> PointLocator... and we can easily change the few places we use it.

Will do.

How about another change: making the PointLocatorBase an argument to
PeriodicBoundaries::neighbor, Elem::periodic_neighbor, etc.?  That's a
little uglier and it's more API breakage, and it's not necessary for
correctness, but it'll allow us to avoid constructing subordinate
PointLocatorTree objects (with empty caches) over and over again,
which ought to be good for efficiency.
---
Roy

On Fri, 22 Apr 2011, Roy Stogner wrote:

> In particular we can see failed assertions (and probably crashes or
> corruption) in multithreaded code that relys on the mesh-provided
> point_locator() incorrectly: MeshFunction, PeriodicBoundaries,
> ExactSolution/ExactErrorEstimator, and RBEIMSystem are likely
> affected.

Okay, after more thorough investigation: only PeriodicBoundaries
support should be affected.  MeshFunction isn't thread-safe, but you
can make a subordinate MeshFunction per-thread to use it safely in
threaded code, and neither the Exact* classes nor the rbOOmit code
seems to do threading at the moment in any case.
---
Roy

On Apr 22, 2011, at 9:17 AM, Roy Stogner wrote:

> How about another change: making the PointLocatorBase an argument to
> PeriodicBoundaries::neighbor, Elem::periodic_neighbor, etc.?  That's a
> little uglier and it's more API breakage, and it's not necessary for
> correctness, but it'll allow us to avoid constructing subordinate
> PointLocatorTree objects (with empty caches) over and over again,
> which ought to be good for efficiency.

Works for me.  That kind of stuff can be arbitrarily complicated: it needs to be fast... and you figure it out once and just leave it alone (until someone changes the API! ;-)

Derek

On Fri, 22 Apr 2011, Derek Gaston wrote:

> On Apr 22, 2011, at 9:17 AM, Roy Stogner wrote:
>
>> How about another change: making the PointLocatorBase an argument to
>> PeriodicBoundaries::neighbor, Elem::periodic_neighbor, etc.?  That's a
>> little uglier and it's more API breakage, and it's not necessary for
>> correctness, but it'll allow us to avoid constructing subordinate
>> PointLocatorTree objects (with empty caches) over and over again,
>> which ought to be good for efficiency.
>
> Works for me.  That kind of stuff can be arbitrarily complicated: it
> needs to be fast... and you figure it out once and just leave it
> alone (until someone changes the API! ;-)

I made yet-a-third API change; some const correctness stuff in
PeriodicBoundaries.

It's all done, working on my initial tests, and committed.  In the
event that all our big test suites pass tonight then I'd like to call
this 0.7.1.  Could you guys run your tests too ASAP?

Thanks,
---
Roy

On Apr 22, 2011, at 12:41 PM, Roy Stogner wrote:

> I made yet-a-third API change; some const correctness stuff in
> PeriodicBoundaries.
> 
> It's all done, working on my initial tests, and committed.  In the
> event that all our big test suites pass tonight then I'd like to call
> this 0.7.1.  Could you guys run your tests too ASAP?

It's probably going to be a bit.  We'll have to deal with the API changes... and we're in the middle of a shit storm right now....

I'll let you know,
Derek

On Fri, 22 Apr 2011, Derek Gaston wrote:

> On Apr 22, 2011, at 12:41 PM, Roy Stogner wrote:
>
>> I made yet-a-third API change; some const correctness stuff in
>> PeriodicBoundaries.
>>
>> It's all done, working on my initial tests, and committed.  In the
>> event that all our big test suites pass tonight then I'd like to call
>> this 0.7.1.  Could you guys run your tests too ASAP?
>
> It's probably going to be a bit.  We'll have to deal with the API changes... and we're in the middle of a shit storm right now....

I just realized that the point_locator API change affects an app code
that really needs to be compatible with multiple libMesh versions
right now, so change of plans there: point_locator() will remain
as-is, but libmesh_deprecated(), and sub_point_locator() will be the
safe-for-threading version.

Hopefully this makes life easier for you too; the other API changes
are less likely to affect user code.
---
Roy