Reduce transient memory allocations in integrate/pw_lin. #2208

thorstenhater · 2023-08-14T15:05:08Z

Another break-out of #2005

Fix logic bug in determining diffusivity, thanks @halfflat
Implement look-back-of-one caching for region/locset thingification
Untangle embed_pwlin integration, save a bit of allocation.

Overall saves ~10% when creating 2048 complex cells of depth
10 in busyring.

NOTE pw_zip_* and pw_over_cable still allocate tons of transients
as they use pw_elements<pw_elements<double>> ie a vector of vectors.
Due to the way this is constructed, dereferencing iterators on these types
returns their contained value not const value&, thus copying the inner
vectors each time. Overall -- despite the frustration and the complexity -- the
remainder doesn't seem worth addressing. For now.

AdhocMan

Looks good overall! Just some minor comment for future maintainability.

AdhocMan · 2023-08-28T08:56:48Z

arbor/cable_cell.cpp

+            case  7: { paint(rc.cables, std::get<init_reversal_potential>(what), rc.region); break; }
+            case  8: { paint(rc.cables, std::get<density>(what), rc.region); break; }
+            case  9: { paint(rc.cables, std::get<voltage_process>(what), rc.region); break; }
+            case 10: { paint(rc.cables, std::get<scaled_mechanism<density>>(what), rc.region); break; }


For new paintable types, adding it here this might get overlooked. std::visit would offer a way to throw a compile time error for such cases, but from the comments elsewhere, I'm assuming this was changed from std::visit for performance reasons here as well. Maybe at least add a default case that throws an error at runtime for unmatched types.

AdhocMan · 2023-08-28T08:57:28Z

arbor/cable_cell.cpp

+            case 0: { place(lc.places, std::get<i_clamp>(what), label); break; }
+            case 1: { place(lc.places, std::get<threshold_detector>(what), label); break; }
+            case 2: { place(lc.places, std::get<synapse>(what), label); break; }
+            case 3: { place(lc.places, std::get<junction>(what), label); break; }


Same here as with the paintings case matching.

…memory

boeschf · 2024-02-27T13:50:45Z

arbor/util/piecewise.hpp

@@ -512,7 +522,7 @@ struct pw_elements<void> {
    auto lower_bound() const { return bounds().first; }
    auto upper_bound() const { return bounds().second; }

-    auto extent(size_type i) const { return extents()[i]; }
+    std::pair<double, double> extent(size_type i) const { return extents()[i]; }


why is auto not enough here?

boeschf · 2024-02-27T14:16:56Z

arbor/util/piecewise.hpp

+        double lower_bound() const { return pw_->extent(*c_).first; }
+        double upper_bound() const { return pw_->extent(*c_).second; }
+
+        const X& value() const { return pw_->cvalue(*c_); };


Ah, so the const_iterator is the culprit? i.e. here the copying is going on when dereferencing the iterator it->lower_bound(), correct? If so, wouldn't having a const specialization for pw_element_proxy<const X> help?

Note that reference is not, in fact, a reference:

using reference = pw_element<X>; reference operator[](difference_type j) const { return (*pw_)[j+*c_]; } reference operator*() const { return (*pw_)[*c_]; } pointer operator->() const { return pointer{(*pw_)[*c_]}; }

thus, [] and * will construct a full pw_element<X>. This is usually not offensive.
BUT, in some cases X=vector<double>, which is obviously an issue. I couldn't get a
working instance where reference = pw_element<X>& or what we'd actually want here
reference = pw_element<X&>.

yes, that's why I proposed to using reference = pw_element_proxy<const X> and specialize

template<typename X> struct pw_element_proxy<const X> { ... };

pw_els<pw_els<>> case.

…memory

boeschf · 2024-03-05T16:48:49Z

arbor/util/piecewise.hpp

@@ -240,16 +259,21 @@ struct pw_elements {

        using value_type = pw_element<X>;
        using pointer = const pointer_proxy<pw_element<X>>;


Suggested change

using pointer = const pointer_proxy<pw_element<X>>;

using pointer = const pointer_proxy<pw_element_proxy<const X>>;

boeschf · 2024-03-05T16:50:25Z

arbor/util/piecewise.hpp

+        double lower_bound() const { return pw_->extent(*c_).first; }
+        double upper_bound() const { return pw_->extent(*c_).second; }


shouldn't be needed anymore, as it is "cheap enough" now to dereference the iterator: it->lower_bound()

boeschf · 2024-03-05T16:52:08Z

arbor/util/piecewise.hpp

+        double lower_bound() const { return pw_->extent(*c_).first; }
+        double upper_bound() const { return pw_->extent(*c_).second; }
+
+        const X& value() const { return pw_->cvalue(*c_); };


same: shouldn't be needed anymore, as it is "cheap enough" now to dereference the iterator

boeschf · 2024-03-05T16:53:45Z

arbor/util/piecewise.hpp

+        double lower_bound() const { return pw_->extent(*c_).first; }
+        double upper_bound() const { return pw_->extent(*c_).second; }


boeschf · 2024-03-05T16:55:22Z

arbor/util/piecewise.hpp

+        double a_right = ai.upper_bound();
+        double b_right = bi.upper_bound();
+        double right = std::min(a_right, b_right);
+        return f(left, right, ai.value());


we can do this again now I believe

Suggested change

double a_right = ai.upper_bound();

double b_right = bi.upper_bound();

double right = std::min(a_right, b_right);

return f(left, right, ai.value());

double a_right = ai->upper_bound();

double b_right = bi->upper_bound();

double right = std::min(a_right, b_right);

return f(left, right, *ai);

thorstenhater · 2024-08-20T08:53:09Z

Split into
#2389
#2388

As discussed in #2208, use a specialized proxy to avoid some allocations.

thorstenhater added 3 commits August 14, 2023 11:06

Use index/get instead of std::visit.

55f93df

Snapshot, doesnt pass

adae932

Fix cases.

4f9d5dd

AdhocMan requested changes Aug 28, 2023

View reviewed changes

thorstenhater added 2 commits October 17, 2023 21:23

Merge remote-tracking branch 'origin/master' into perf/reduce-pw-lin-…

ec90987

…memory

Complain -- ie terminate -- on unknown cases.

78f821b

boeschf reviewed Feb 27, 2024

View reviewed changes

thorstenhater added 3 commits March 5, 2024 13:44

Also add proxy for constant iterator, hoping we'll save the

7ef78b6

pw_els<pw_els<>> case.

Merge remote-tracking branch 'origin/master' into perf/reduce-pw-lin-…

1fecaa1

…memory

Only print profile if we can.

37b4d81

boeschf reviewed Mar 5, 2024

View reviewed changes

thorstenhater mentioned this pull request Aug 20, 2024

Make const element proxy. #2388

Merged

thorstenhater closed this Aug 20, 2024

thorstenhater added a commit that referenced this pull request Sep 10, 2024

Make const element proxy. (#2388)

a292557

As discussed in #2208, use a specialized proxy to avoid some allocations.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce transient memory allocations in integrate/pw_lin. #2208

Reduce transient memory allocations in integrate/pw_lin. #2208

thorstenhater commented Aug 14, 2023 •

edited

Loading

AdhocMan left a comment

AdhocMan Aug 28, 2023

AdhocMan Aug 28, 2023

boeschf Feb 27, 2024

boeschf Feb 27, 2024

thorstenhater Feb 27, 2024

boeschf Mar 5, 2024 •

edited

Loading

boeschf Mar 5, 2024

boeschf Mar 5, 2024

boeschf Mar 5, 2024

boeschf Mar 5, 2024

boeschf Mar 5, 2024

thorstenhater commented Aug 20, 2024

		@@ -240,16 +259,21 @@ struct pw_elements {

		using value_type = pw_element<X>;
		using pointer = const pointer_proxy<pw_element<X>>;

		double lower_bound() const { return pw_->extent(*c_).first; }
		double upper_bound() const { return pw_->extent(*c_).second; }

Reduce transient memory allocations in integrate/pw_lin. #2208

Reduce transient memory allocations in integrate/pw_lin. #2208

Conversation

thorstenhater commented Aug 14, 2023 • edited Loading

AdhocMan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boeschf Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorstenhater commented Aug 20, 2024

thorstenhater commented Aug 14, 2023 •

edited

Loading

boeschf Mar 5, 2024 •

edited

Loading