Interfacing librsb into Epetra

[michelemartone]

rsb.hpp.txt
Epetra_CrsMatrix.h.txt
Epetra_CrsMatrix.cpp.txt
@mhoemmen @maherou

Hi. After my talk at EUROTUG'16 on Apr. 18-20 2016 near Munich [ https://www.mhpc.mw.tum.de/index.php?id=38 ] and subsequent discussion with Michael and Mark I've been encouraged to propose an incremental integration of the sparse matrix type RSB (provided by librsb [ http://librsb.sourceforge.net/ ]) into Epetra.
So following such a minimalist approach I have modified a few lines in Epetra_CrsMatrix (from trilinos-12.6.1) to use librsb for: MV/MTV/SV/SM, in code delimited by the HAVE_EPETRA_RSB
preprocessor symbol; and constructs in Epetra_CrsMatrix::FillComplete and in the copy constructor.
So the prototype I am providing creates an RSB copy of the matrix just for the purposes of using it in the operations above.

I am also working on a proper "Epetra_RsbMatrix" class, but let's first see if this one here is able to sustain some tests for the above operations.

The way I suggest to build this is:

• get my / michele martone's GPG key

  gpg --search EF1258B8

• download librsb from

  https://sourceforge.net/projects/librsb/files/librsb-1.2.0-rc3.tar.gz/download

  download its signature from

  https://sourceforge.net/projects/librsb/files/librsb-1.2.0-rc3.tar.gz.asc/download

  verify signature

  gpg --verify librsb-1.2.0-rc3.tar.gz.asc

  build librsb

  tar xzf librsb-1.2.0-rc3.tar.gz
  cd librsb-1.2.0-rc3
  ./configure CFLAGS="-O3 -fPIC" FCFLAGS="-O3 -fPIC" --prefix=/opt/librsb
  make

  quick check

  make qtests

  long check (I recommend to skip it)

  make tests
  make install
• use attached version of Epetra_CrsMatrix.cpp and Epetra_CrsMatrix.h
  and put attached rsb.hpp (the Epetra-agnostic C++ wrapper to librsb)
  into e.g. /opt/librsb/include/
• build Epetra including e.g. /opt/librsb/bin/librsb-config --I_opts
• link with e.g. /opt/librsb/bin/librsb-config --ldflags -fopenmp -lm -lz

I attach the source files I have used, with ".txt" appended to the filename.
The rsb.hpp file is meant to become part of a future release of librsb.

I am new here, so if there is a more practical approach to follow to provide this proof-of-concept test, please suggest it to me.

[mhoemmen]

@michelemartone Thanks so much for posting this here!

How would you prefer to use RSB in Epetra? Would you prefer for it to happen "behind the scenes," invisibly to the user, if they enable the RSB option at configuration time? Would you prefer users to select it explicitly in their code via a run-time option (e.g., to FillComplete), or would you prefer a different class name (Epetra_RsbMatrix)?

There might be some advantage to exposing the RSB data structure to users, if they know how to fill into it efficiently. On the other hand, I think your intention in part is to hide the data structure, so that applications that fill into Epetra_CrsMatrix can get performance benefits in their solves. Is that right?

[michelemartone]

@mhoemmen

I would suggest now having it behind Epetra_CrsMatrix just for testing purposes (e.g. correctness and performance of these operations) --- so "hiding" just for the time being, temporarily.

After you give me the OK with this minimal patch (i.e. it passes a number of Trilinos tests), my next step can be sending you an explicit Epetra_RsbMatrix class implementation --- I have a prototype which will soon be testable by you as well.

[mhoemmen]

@michelemartone Before integrating these changes into Epetra for production, I think it would be better to have a complete patch that implements what you intend to implement. I'm not keen on temporary experimental code going into Epetra, even if it's disabled by default. That's why I was asking what your end goal is with these changes. Is your final intention to have a separate class, Epetra_RsbMatrix? If so, then I think we should wait until you're done with that before merging code into Epetra for production. If your end goal is to use RSB in Epetra_CrsMatrix in a way that's invisible to users, then let's do that.

I'm being extra careful because many Trilinos packages and applications depend on Epetra and use it heavily. Furthermore, Epetra is not changing much (if at all), and will not change much for a long time. Thus, it's totally OK to spend some time refining this patch until it reaches a point at which you are satisfied and consider it safe for production use.

[michelemartone]

@mhoemmen Having a minimal patch like the one I have provided, "hiding" RSB in Epetra_CrsMatrix, is what Michael has suggested in Munich as a preliminary first step for being able to test correctness and performance of RSB. So this is possible now with the three files I have provided to you.

The end goal would be to obtain Epetra_RsbMatrix --- that I will provide you soon, in the way you suggest. Then I will also add the new files to this ticket, unless you suggest otherwise.

[mhoemmen]

@jwillenbring suggested that you could start a public topic branch. That would let everyone try out your modifications, without adding anything yet to the main Trilinos repository. Patches are OK, but it's nicer to use Github features -- that makes applying and modifying patches and adding comments much easier.

[michelemartone]

I'm completely new to Github, could you please provide me a starting point or existing example of such a way of contributing ?

[jwillenbring]

You will want to fork the Trilinos repository

https://help.github.com/articles/fork-a-repo/

You will then want to create a branch on your fork, and make the changes you want to make

https://git-scm.com/book/en/v2/Git-Branching-Basic-Branching-and-Merging

When you are done making changes, you will want to issue a pull request against the Trilinos repository. Depending on a few things, it may make sense to create a copy of your feature branch on the main Trilinos repo to try things prior to applying the changes to the develop branch.

https://help.github.com/articles/using-pull-requests/

[michelemartone]

Thanks for the links.

I followed what you suggest.

I've created my fork of Trilinos:
https://github.com/michelemartone/Trilinos

Then I've branched it, and I issued a pull request:
#503

Please give a look to it --- instructions as in the original ticket:
#431

I'm waiting for your comments.

[mhoemmen]

@michelemartone Thanks for doing this! Thanks also to @jwillenbring for the instructions. I'll take a look.

[mhoemmen]

@michelemartone Comments on the changes, in order of decreasing importance:

1. What happens if users modify the matrix's entries after FillComplete has been called? (Epetra allows this.) Is there a way for Epetra_CrsMatrix to update the RSB data structure with these changes? (If not, it would be better to treat RSB as an Epetra_RowMatrix or Epetra_Operator. Epetra has no equivalent to Tpetra::CrsMatrix::resumeFill that would signal the end of a phase of changing the matrix's entries.)
2. Could you clarify the comment "FIXME: UnitDiagonal and Upper args ignored" on line 3746 of Epetra_CrsMatrix.cpp? (You could just defer to the existing CSR implementation for cases that librsb does not implement.)
3. Does librsb have a "finalize" method corresponding to rsb_lib_init? If not, should it have one? (For example, does rsb_lib_init allocate global state that needs deallocation?)
4. Could you clarify #define RSBP_WANT_CXX11 1 on line 62 of Epetra_CrsMatrix.h? (It's totally OK for your use of librsb in Epetra to require C++11. I just want to check if that's what you meant. If so, I can help you add CMake logic to Epetra so that it raises a configure-time error if C++11 is not enabled.)
5. Macros (such as EPETRA_RSB_NEW) can have line breaks; just use a backslash as the last character of the line.

It looks like the changes are not any more invasive than, say, those for CASK. That makes me feel better about including them in mainline Epetra. That leaves three things to do:

1. Address any issues in the above comments
2. Add CMake TPL logic for librsb (so we can say "Trilinos_ENABLE_librsb=ON" etc.)
3. Test Epetra and downstream classes with the librsb TPL enabled

We can help with all of these, in particular (2) and (3).

[michelemartone]

What you have seen is the "minimal librsb interfacing" proposed by Michael back in Munich at EUROTUG.
Its purpose is only to enable you (Trilinos devs) to check librsb's SPMV goodness/performance (=the main motivator behind it).

His plan was that after a first OK on your side, I could pass you a further, 1.7KLOC file that I have ready on my disk, with an entire: "class Epetra_RsbMatrix: public Rsb_Matrix, Epetra_CrsMatrix" implementation inside, based on the Rsb_Matrix you have in rsb.hpp.

The rsb.hpp file is intended to be part of librsb (yet unreleased, you have now this beta), but it is with the file with class Epetra_RsbMatrix that I aim to seriously contribute into Trilinos.

Let me answer 1-5 limitedly to my Epetra_CrsMatrix:

1. Modification of entries not meant to work here (but in the Epetra_RsbMatrix yes).
2. My Epetra_CrsMatrix class creates an RSB clone and then uses that. This means the e.g. SV routine works on that RSB internal version and ignores the CRS vectors.
3. Yes, there is a rsb_lib_exit() cleaning a tiny amount of memory.
4. I've developed this class using C++11. If I can make C++11 as a requirement like you say it's perfect.
5. Ok, coming versions will have shorter lines.

The Epetra_RsbMatrix class file prototype is ready when you need it, I guess after testing the above.

Sure, CMake logic in (2) and your testing in (3) are what is needed for Epetra_RsbMatrix !

[mhoemmen]

Its purpose is only to enable you (Trilinos devs) to check librsb's SPMV goodness/performance (=the main motivator behind it).

@michelemartone I think you folks are the best judges of librsb performance. If it's significantly faster than the current implementation for your applications, then it's fast enough :-) Was your intention that we work on optimizing librsb performance together? If so, let's discuss that more over e-mail.

[mhoemmen]

My Epetra_CrsMatrix class creates an RSB clone and then uses that. This means the e.g. SV routine works on that RSB internal version and ignores the CRS vectors.

If I understand correctly, your comment in the code suggests that your RSB implementation is incomplete. It does not correctly handle the cases where the matrix has an implicitly stored unit diagonal, or where it is upper triangular instead of lower triangular. Typical LU factorizations (including incomplete factorizations) store the L factor with an implicit unit diagonal, and U is upper triangular. The most common reason to need to do sparse triangular solves with Epetra_CrsMatrix is to apply the result of an incomplete LU factorization, and the next most common reason is to apply the result of a complete LU factorization. The Amesos and Ifpack packages will certainly want to exercise this capability. Thus, it is unlikely that downstream tests will pass, unless SV respects the UnitDiagonal and Upper arguments.

[mhoemmen]

Yes, there is a rsb_lib_exit() cleaning a tiny amount of memory.

You could always set an atexit() hook to call rsb_lib_exit().

[mhoemmen]

If I can make C++11 as a requirement like you say it's perfect.

When we set up the librsb TPL, we would add CMake logic to Epetra to set a macro if the TPL is enabled. That logic would test whether C++11 is enabled, and stop with a configure-time error if it is not. That's the easiest way to do this. I'll be happy to help you set that up.

[michelemartone]

@mhoemmen : Sure, hooking rsb_lib_exit() with atexit() is a good idea for Epetra_RsbMatrix.
@mhoemmen : On large (>1e6 nnz) symmetric matrices librsb shall be already significantly faster than CSR in SPMV. So I'd say speaking about optimizing further is for later. (I thinks there's definitive space for improvement, but it's already fast ;- )
@mhoemmen : librsb as such supports unit diagonal and either upper or lower triangular, and so will Epetra_RsbMatrix.. This minimal interfacing so far, not. [BTW librsb's SV can expose some parallelism--which is good]

[jwillenbring]

I think this is on the track you are on anyway, @mhoemmen, but we can have this new feature depend on C++11, but we cannot have Epetra depend on C++11, and any Epetra features that depend on C++11 should not be enabled by default.

[michelemartone]

@jwillenbring : I suggest to make now depend Epetra_RsbMatrix on C++11, then in the future we can see if that's avoidable (highly probably yes -- 99% of the code is in librsb's C code anyways).

[mhoemmen]

I think this is on the track you are on anyway, @mhoemmen, but we can have this new feature depend on C++11, but we cannot have Epetra depend on C++11, and any Epetra features that depend on C++11 should not be enabled by default.

@jwillenbring That's exactly what I meant -- thanks for restating more clearly :-) librsb would not be enabled by default. It would be a TPL. Epetra features that depend on this TPL may use C++11, because the TPL would not be enabled by default. It would nevertheless still be good to avoid use of C++11 features in Epetra, even if they depend on the TPL.

[mhoemmen]

@jwillenbring : I suggest to make now depend Epetra_RsbMatrix on C++11, then in the future we can see if that's avoidable (highly probably yes -- 99% of the code is in librsb's C code anyways).

It would be nicer if we could limit all C++11 code to librsb. However, I'll accept C++11 code in those parts of Epetra that depend on the librsb TPL. It's important not to let C++11 code leak out into parts of Epetra that do not depend on the librsb TPL, though.

[github-actions]

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity.
If you would like to keep this issue open please add a comment and remove the MARKED_FOR_CLOSURE label.
If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE.

[github-actions]

This issue was closed due to inactivity for 395 days.