Add Distributed VectorCache and use it for intermediate vector in and adapt `apply_uses_initial_guess` in Schwarz #1688

yhmtsai · 2024-10-09T12:40:05Z

This adds a cache/workspace for the intermediate vector in advanced_apply of Schwarz.
Also, it makes apply_uses_initial_guess rely on the local_solver->apply_uses_initial_guess().
We might need to revisit this function after adding refining step in Schwarz.

MarcelKoch · 2024-10-09T12:53:35Z

core/distributed/preconditioner/schwarz.cpp


+template <typename ValueType, typename LocalIndexType, typename GlobalIndexType>
+template <typename VectorType>
+void Schwarz<ValueType, LocalIndexType, GlobalIndexType>::set_cache_to(


You only need this function once, right? Then I would prefer to just add this directly, it's not that much IMO.

MarcelKoch · 2024-10-09T12:54:21Z

include/ginkgo/core/distributed/preconditioner/schwarz.hpp

+     *            (could also be done with writing `operator=` and copy
+     *            constructor of the enclosing class by hand)
+     */
+    mutable struct cache_struct {


I think this should be a DenseCache instead.

I think it does not support distributed::Vector.

you're right. Perhaps it would make sense to add VectorCache, instead of using the cache construct.

pratikvn

I agree with the Marcel that it can be a DenseCache instead of cache_struct. But otherwise looks good to me. I approve it as Marcel has already requested that change.

MarcelKoch · 2024-11-04T09:39:35Z

include/ginkgo/core/matrix/dense.hpp

    friend class SparsityCsr<ValueType, int64>;
    friend class Dense<to_complex<ValueType>>;
    friend class experimental::distributed::Vector<ValueType>;
+    friend class gko::detail::VectorCache<ValueType>;


why is this necessary?

Because distributed::Vector holds the Dense object not Dense pointer, I need the constructor to construct the object directly and then use the move_to

ah, I might use the unique_ptr.release from create function

No it's fine, I just got confused. I thought this was in the distributed::Matrix.

pratikvn

Nice work! LGTM!

core/base/vector_cache.cpp

pratikvn · 2024-11-04T09:33:50Z

core/base/vector_cache.cpp

+        // handle locally to eliminate the mpi call
+        std::cout << "local diff" << comm.rank() << std::endl;
+        vec->local_ =
+            std::move(gko::matrix::Dense<ValueType>(exec, local_size));


nit: use the create function instead ?

We hold the object not the pointer.
However, I can create the unique pointer and release the object out actually, right?

do you think it is neccessary to do unique_ptr through the create function?
I slightly prefer the constructor to avoid calling release from unique_ptr, but it requires the friend declration in dense such that VectorCache can use Dense constructor

No, It is a only a nit from my side. Using the constructor should also be okay.

pratikvn · 2024-11-04T09:34:19Z

core/base/vector_cache.cpp

+    } else if (vec->get_local_vector()->get_size() !=
+               template_vec->get_local_vector()->get_size()) {
+        // handle locally to eliminate the mpi call
+        vec->local_ = std::move(gko::matrix::Dense<ValueType>(


nit: use the create function instead ?

pratikvn

Some fixes needed

pratikvn · 2024-11-04T09:44:13Z

include/ginkgo/core/base/vector_cache.hpp

+#if GINKGO_BUILD_MPI
+
+
+namespace gko {


I think it might make sense to put this into the experimental and distributed namespaces.

I move it into gko::experimental::distributed::detail

include/ginkgo/core/base/vector_cache.hpp

include/ginkgo/core/distributed/preconditioner/schwarz.hpp

Co-authored-by: Pratik Nayak <pratikvn@protonmail.com>

sonarqubecloud · 2024-11-05T23:04:48Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

…intermediate vector in and adapt `apply_uses_initial_guess` in Schwarz This PR adds distributed VectorCache and uses it as the workspace in Schwarz to avoid alloc/free memory operation. Related PR: ginkgo-project#1688

yhmtsai added the 1:ST:ready-for-review This PR is ready for review label Oct 9, 2024

yhmtsai requested review from a team and pratikvn October 9, 2024 12:40

yhmtsai self-assigned this Oct 9, 2024

ginkgo-bot added reg:testing This is related to testing. mod:core This is related to the core module. type:preconditioner This is related to the preconditioners labels Oct 9, 2024

MarcelKoch requested changes Oct 9, 2024

View reviewed changes

pratikvn approved these changes Oct 13, 2024

View reviewed changes

MarcelKoch added this to the Ginkgo 1.9.0 milestone Oct 17, 2024

yhmtsai force-pushed the schwarz_update branch 2 times, most recently from 8948a5d to 4ed9711 Compare November 4, 2024 00:14

yhmtsai requested review from MarcelKoch and pratikvn November 4, 2024 09:18

MarcelKoch approved these changes Nov 4, 2024

View reviewed changes

pratikvn approved these changes Nov 4, 2024

View reviewed changes

pratikvn requested changes Nov 4, 2024

View reviewed changes

yhmtsai added 3 commits November 4, 2024 11:13

add workspace for intermediate data

dc94070

apply_uses_initial_guess return the local solvers' value

4c10c18

use vector cache

030bc70

yhmtsai force-pushed the schwarz_update branch from 4ed9711 to 030bc70 Compare November 4, 2024 10:13

yhmtsai requested a review from pratikvn November 4, 2024 14:55

pratikvn approved these changes Nov 4, 2024

View reviewed changes

include/ginkgo/core/distributed/preconditioner/schwarz.hpp Outdated Show resolved Hide resolved

yhmtsai force-pushed the schwarz_update branch from fc33d21 to f8b9f57 Compare November 4, 2024 21:51

yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Nov 4, 2024

yhmtsai and others added 2 commits November 5, 2024 10:57

move VectorCache into experimental::distributed

6171124

Co-authored-by: Pratik Nayak <pratikvn@protonmail.com>

fix typo of rcm

8be4221

yhmtsai force-pushed the schwarz_update branch from f8b9f57 to 8be4221 Compare November 5, 2024 09:57

yhmtsai changed the title ~~add workspace for intermediate vector in and adapt apply_uses_initial_guess in Schwarz~~ Add Distributed VectorCache and use it for intermediate vector in and adapt apply_uses_initial_guess in Schwarz Nov 5, 2024

yhmtsai merged commit 7b04094 into develop Nov 5, 2024
8 of 11 checks passed

yhmtsai deleted the schwarz_update branch November 5, 2024 14:02

		#if GINKGO_BUILD_MPI


		namespace gko {

Add Distributed VectorCache and use it for intermediate vector in and adapt apply_uses_initial_guess in Schwarz #1688

Add Distributed VectorCache and use it for intermediate vector in and adapt apply_uses_initial_guess in Schwarz #1688

Uh oh!

Conversation

yhmtsai commented Oct 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pratikvn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pratikvn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pratikvn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Nov 5, 2024

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add Distributed VectorCache and use it for intermediate vector in and adapt `apply_uses_initial_guess` in Schwarz #1688

Add Distributed VectorCache and use it for intermediate vector in and adapt `apply_uses_initial_guess` in Schwarz #1688