Itk: Some VNL solvers are not thread-safe

[nslay]

Description

Contrary to v3p's README, some VNL netlib optimizers still have static variables declared in functions. The differences are especially easy to spot when comparing the ITK repository with the VXL repository.

https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/VNL/src/vxl/v3p/netlib/README.md

The v3p_netlib library contains updated netlib code converted with f2c in a thread-safe manner. Sources have been modified to use the v3p_*.h headers in this directory. Types and function names have been mangled to start with a v3p_netlib_ prefix. This distinguishes them from other netlib libraries and keeps the code grouped and isolated.

Example non-thread-safe netlib code in ITK:
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/VNL/src/vxl/v3p/netlib/opt/lbfgsb.c

Example thread-safe netlib code in VXL
https://sourceforge.net/p/vxl/svn/HEAD/tree/trunk/v3p/netlib/opt/lbfgsb.c

Here's a thread-safe VNL netlib library fix announcement on the VXL mailing list:
https://sourceforge.net/p/vxl/mailman/vxl-maintainers/thread/44CF9607.6090804%40kitware.com/

Maybe someone re-ran f2c and clobbered those changes in ITK?

Steps to Reproduce

1. In several threads, instantiate one thread-local vnl_lbfgsb solver (vnl_lbfgs is actually thread-safe!).
2. Run the solver in each thread and hope it crashes by chance.
3. Or... check that deterministic code is not giving the same output for the same inputs when running multiple threads (example attached).

See attached.
LetsCrashVNL.zip

Try the following:
vnl_lfbgs and observe that it works (line 39)
vnl_lfbgsb and observe that it's not working (lots of different answers).
uncomment num_threads(1) (line 32) directive and observe vnl_lfbgsb works now.

When it works, there should be no "Different answer" messages as this is deterministic code being fed the same input over and over again.

Expected behavior

When presenting the user with a class as the interface to a solver, I would expect isolation between multiple instantiations of the solver.

Actual behavior

Using one instantiation of a solver may have side effects on a different instantiation of the solver.

Reproducibility

Random. May even appear to work!

Versions

ITK 4.13, but I've seen mysterious crashes when using OpenMP with VNL solvers (one per thread) as far back as ITK 4.7.

Environment

Windows 7, 10, Ubuntu 14.04, FreeBSD 11.2

Additional Information

https://sourceforge.net/p/vxl/mailman/vxl-maintainers/thread/44CF9607.6090804%40kitware.com/
https://sourceforge.net/p/vxl/svn/HEAD/tree/trunk/v3p/netlib/opt/lbfgsb.c

This problem may not be unique to vnl_lbfgsb. This problem will obviously affect users of std::thread, pthreads, etc... L-BFGS-B is especially nice as it performs constrained optimization while L-BFGS does not.

[hjmjohnson]

@nslay vxl has moved to github several years ago. Would you please reference the code on the github vxl site?

https://github.com/vxl/vxl

[nslay]

Thanks for the prompt response!

I'm quite surprised because the github VXL repository has the same discrepancy as ITK while the sourceforge one is thread-safe. Here is the offending non-threadsafe lbfgsb.c code:

https://github.com/vxl/vxl/blob/master/v3p/netlib/opt/lbfgsb.c

I've downloaded the sourceforge one and attached it.

lbfgsb.txt

Sorry, github doesn't support C files. I renamed it with a .txt extension.

I wonder what happened in the migration to github!

[nslay]

The very first commit on github is threadsafe. I'll try to find where it changed.
https://github.com/vxl/vxl/blob/ae9c7b5a9253f4ac14f5b247a816b5a1a559d80a/v3p/netlib/opt/lbfgsb.c

[hjmjohnson]

My guess is : https://github.com/vxl/vxl/commit/3cdc40c20d46989c4555f747f28f17ba65e7761a

[nslay]

The massive amount of static variables got reintroduced here (to fix another bug):
https://github.com/vxl/vxl/commit/3cdc40c20d46989c4555f747f28f17ba65e7761a#diff-b59cc8496f016fff270b123014286c74

[nslay]

I'm glad VXL is still alive and kicking! It's my favorite numerics library. I thought it was abandoned or was just extremely mature when I saw that the last commit was in 2013 on sourceforge.

Google pointed me to VXL's sourceforge (first result). Github wasn't anywhere in the search results!

[dzenanz]

If somebody has write access to vxl.sourceforge.net, they could add a link to GitHub and state that VXL has moved there.

[hjmjohnson]

I think that would be @bradking

[nslay]

What I don't understand is why f2c emits static variables for otherwise local variables in the original Fortran code. I'd expect this for, say, Fortran's common blocks (yuck!), but not these variables!

Does Fortran have some sort of guarantee about the states of local variables in functions called multiple times?

[bradking]

I've opened vxl/vxl#555 to discuss the future of the vxl homepage.

[hjmjohnson]

@dzenanz FYI: https://github.com/vxl/vxl/issues/555 . VXL homepage moved to github.

[hjmjohnson]

@nslay f2c probably does not emit static, but f2c often produces a draft of code that subsequently requires modification in order to meet coding standards/ performance/linkage constraints not supported in the original implementation.

It would be fabulous if you could provide a pull request for this codebase that fixes the problem you identified.

both f2c, and the developers that attempted to fix this file are imperfect.

Thanks,
Hans

[nslay]

Hi Hans,
ITK/VXL developers are not perfect!? I am outraged!

OK. I will work on this problem. I also apparently need to tidy up ITKIOOpenSlide!

Nate

[hjmjohnson]

@nslay 👍 It was almost certainly me who made those static. Just be careful not to blindly move the old code forward. It may fix thread safety at the expense of re-introducing other problems.

Thanks for looking into this.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[pieper]

hey, shouldn't this be reopened? Sounds like it's not fixed and people have told me it's still and issue. @hjmjohnson @thewtex

[nslay]

Yeah yeah, I intended to fix this. I will have another look this weekend.

[hjmjohnson]

@nslay THANK YOU!!!

You may want to look at recent upstream changes to this code base L-BFGS-B-C for fixing this+potentially other bugs. If you find additional bugs, perhaps we should submit them upstream.

[pieper]

@nslay, ditto on @hjmjohnson's THANK YOU!!!

We have some code that can be used to help test when you are ready.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.