Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid divisions/calulcation of steps #685

Merged
merged 6 commits into from Jun 14, 2017
Merged

Conversation

@jakobj
Copy link
Contributor

@jakobj jakobj commented Mar 20, 2017

This PR implements the changes suggested by Harald Servat to reduce run time by avoiding divisions and adding additional member in Event and Time objects that store frequently used values instead of calculating them from the other members on every invocation. Preliminary benchmarks show an increase of ~15% on JUQUEEN. Plots will follow.
I suggest @heplesser @apeyser @terhorstd as reviewers. If Harald Servat could also have a look that would be great, unfortunately I don't know his GitHub handle.

@jakobj jakobj force-pushed the jakobj:enh/avoid_divisions branch from aba06cf to d05335a Mar 20, 2017
@apeyser
Copy link
Contributor

@apeyser apeyser commented Mar 20, 2017

Please see my poster from two years ago with @wschenck on why you shouldn't cache values like this.

On the other hand, inverting constants or statics and using them as multiplicative values is good -- as long as you aren't pushing the envelope of the floating point ranges and losing precision.

@jakobj
Copy link
Contributor Author

@jakobj jakobj commented Mar 20, 2017

Thanks for the input @apeyser. If I understand you correctly, I should remove the steps member in the Time class, the rest can stay?

@heplesser
Copy link
Contributor

@heplesser heplesser commented Mar 21, 2017

@apeyser Is you poster available online somewhere? It would be very useful for reference.

@apeyser
Copy link
Contributor

@apeyser apeyser commented Mar 21, 2017

@heplesser https://juser.fz-juelich.de/record/256297/files/poster.pdf , just search alexander peyser sfn 2015 on google scholar

@jakobj Yes -- I think storing the inversions is a very good idea -- someone should check the boundary conditions though, whether you lose precision for extreme values (probably not, but needs checked). For the statics, particularly good --- I didn't do it originally only because they changes where already so big, I didn't want to confuse matters (if you look, some of it is that way already).

@jakobj
Copy link
Contributor Author

@jakobj jakobj commented Mar 27, 2017

Here are some benchmarks of the proposed changes. Sorry for the labels: div3 = all changes as proposed, div4 = all changes as proposed without the extra member in the time objects. These seem to differ from your data @apeyser, as the additional member in Time objects leads to a decrease in runtime, not an increase. something we should discuss in today's meeting.
JUQUEEN_master_jemalloc_git92d53f9_master_jemalloc_nodiv3_git0bd90b7_master_jemalloc_nodiv4_gitc2bd0b4_bympiprocesses.pdf

@apeyser
Copy link
Contributor

@apeyser apeyser commented Mar 28, 2017

@jakobj What's the difference between A & B?

@jakobj
Copy link
Contributor Author

@jakobj jakobj commented Mar 28, 2017

Right, sorry. (A) is run time, (B) is build time.

@apeyser
Copy link
Contributor

@apeyser apeyser commented Apr 5, 2017

@jakobj
So, to summarize: there's a small but significant (< 20%) improvement in run time by removing divisions and adding a caching member in runtime; but the difference between adding and not adding the caching of the step time is insignificant. In build time, there a small improvement by the current code (10%?) and the difference between div3 and div4 is insignificant.

Is this correct? If so, I'd say it worth removing the division but not adding the complexity of the caching. I don't think this conflicts with my results -- in my results, we removed two extra members (both steps and ms), and Wolfram had eliminated all together the time object construction for event dispatch. (That was the single biggest problem, that the fat time object was being passed, constructed, extracted, reconstructed with an integer + floating point division, and then repeated).

These changes wouldn't affect the event dispatch, because now the code path is completely gone, and the conversion from int tics to int steps is a fraction of the computational cost from int tics to int steps and float ms. Thus the two biggest problems are unaffected here.

I'm though curious about B and why the build time increases at all. That shouldn't happen from what I know of the functionality. Something's fishy. Maybe during building time there's a lot of operations on time, so most steps_ computes are thrown away, while during runtime it's a large number of identical times.

@@ -66,6 +68,8 @@ nest::Archiving_Node::Archiving_Node( const Archiving_Node& n )
, beta_Ca_( n.beta_Ca_ )
, synaptic_elements_map_( n.synaptic_elements_map_ )
{
tau_minus_inv_ = 1. / tau_minus_;

This comment has been minimized.

@apeyser

apeyser Apr 5, 2017
Contributor

This should be copied directly from const Archiving_Node& n rather than recomputed from the base value.

@@ -47,6 +47,7 @@ Event::Event()
, offset_( 0.0 )
, w_( 0.0 )
{
steps_ = stamp_.get_steps();

This comment has been minimized.

@apeyser

apeyser Apr 5, 2017
Contributor

Shouldn't this just be in the initializer as steps(Time::step(0))?

This comment has been minimized.

@apeyser

apeyser Apr 5, 2017
Contributor

Actually, that should just be steps(0) I think -- with my PR #699 .

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

I agree, use initializer steps_( 0 ), but see suggestion for name change below.

@@ -473,6 +485,7 @@ class Time
{

This comment has been minimized.

@apeyser

apeyser Apr 5, 2017
Contributor

This is where you're getting extra complexity that will also cost performance: anyplace where you have a loop around time += n secs you're going to compute unneeded steps. It's better to cache as in event.h, where you know you need to keep the value, and not everywhere in the code where, if you're lucky, it all balances out to zero, or gives you sometimes a gain, and sometimes a loss.

This comment has been minimized.

@heplesser

heplesser Apr 7, 2017
Contributor

@apeyser I am a bit confused---did this comment end up in the wrong place?

This comment has been minimized.

@apeyser

apeyser Apr 7, 2017
Contributor

In commit d05335a it appears in the right place... but not in the PR discussion it seems. To reduce the confusion I created, I was referring to calculate_steps() in Time(tic_t) and Time(). By caching there, under some usage patterns, you'll have a huge number of caching steps that are never used. For example, in vector<Time> or in temporaries created in computations.

But I believe downstream we've already agreed to just not do this, and do lazy evaluation in the Event class.


// round tics up to nearest step
// by adding TICS_PER_STEP-1 before division
return ( tics + Range::TICS_PER_STEP_RND ) / Range::TICS_PER_STEP;

This comment has been minimized.

@apeyser

apeyser Apr 5, 2017
Contributor

But here would be a good place to use an inverted value

@@ -537,6 +562,9 @@ class Time
{
return ld_round( ms * Range::STEPS_PER_MS );
}

private:
unsigned long steps_;

This comment has been minimized.

@apeyser

apeyser Apr 5, 2017
Contributor

Otherwise, if I were going complicated, I wouldn't precompute steps, but cache the value computed in calculate_steps, which would only be computed for get_steps, with an invalid marker in steps_ to tell me when I needed to compute. If the caching is worth it, that's a much more efficient way to do so.

But I think there's probably places in the code where time is being repeatedly converted to steps (like in event), and simply converting and storing the steps is what you really want.

This comment has been minimized.

@apeyser

apeyser Apr 5, 2017
Contributor

And C is memory usage for the extra members? I wouldn't expect to see anything here -- the issue wouldn't be memory usage, but the extra demand on bandwidth between RAM and caches, and multiple registers used unnecessarily, plus confusing the optimizer by operating on multiple operands that are glued together.

@apeyser apeyser requested a review from gtrensch Apr 5, 2017
Copy link
Contributor

@heplesser heplesser left a comment

@jakobj Mostly fine, but some improvements required, see in the text. And it would be good to understand why building becomes slower.

@@ -45,6 +45,7 @@ STDPPLHomCommonProperties::STDPPLHomCommonProperties()
, alpha_( 1.0 )
, mu_( 0.4 )
{
tau_plus_inv_ = 1. / tau_plus_;

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

This should also be initialized in the initializer list

, tau_plus_( 20.0 )
, tau_plus_inv_( 1. / tau_plus_ )
@@ -65,6 +66,7 @@ STDPPLHomCommonProperties::set_status( const DictionaryDatum& d,
CommonSynapseProperties::set_status( d, cm );

updateValue< double >( d, "tau_plus", tau_plus_ );
tau_plus_inv_ = 1. / tau_plus_;

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

This needs division-by-zero protection:

if ( updateValue< double >(d, "tau_plus", tau_plus_ ) )
{
  if ( tau_plus_ > 0. )
  { 
    tau_plus_inv_ = 1. / tau_plus_; 
  }
  else
  {
    throw BadProperty( "tau_plus_ > 0. required." );
  }
}

This comment has been minimized.

@apeyser

apeyser Apr 7, 2017
Contributor

Something to remember for further reviews and checks, since apparently that's slipped in, in many places earlier and only became apparent now.

@@ -91,6 +91,7 @@ class STDPPLHomCommonProperties : public CommonSynapseProperties

// data members common to all connections
double tau_plus_;
double tau_plus_inv_;

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

Add comment //!< 1 / tau_plus_ for efficiency

@@ -50,6 +50,8 @@ nest::Archiving_Node::Archiving_Node()
, beta_Ca_( 0.001 )
, synaptic_elements_map_()
{
tau_minus_inv_ = 1. / tau_minus_;
tau_minus_triplet_inv_ = 1. / tau_minus_triplet_;

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

Should be done in initializer list, see above.

@@ -47,6 +47,7 @@ Event::Event()
, offset_( 0.0 )
, w_( 0.0 )
{
steps_ = stamp_.get_steps();

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

I agree, use initializer steps_( 0 ), but see suggestion for name change below.

@@ -309,6 +309,8 @@ class Event
*/
Time stamp_;

unsigned long steps_;

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

For clarity of what this variable represents, I would suggest to call it stamp_steps_. It should also be documented with a doxygen comment, pointing out that we add it for efficiency.

@@ -162,7 +165,7 @@ Time::fromstamp( Time::ms_stamp t )
// intended ones.
tic_t n = static_cast< tic_t >( t.t * Range::TICS_PER_MS );
n -= ( n % Range::TICS_PER_STEP );
long s = n / Range::TICS_PER_STEP;
long s = n * Range::TICS_PER_STEP_INV;
double ms = s * Range::MS_PER_STEP;

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

s and ms could both be const.

This comment has been minimized.

@apeyser

apeyser Apr 6, 2017
Contributor

What we need here is just MS_PER_TIC (MS_PER_STEP * STEP_PER_TIC).

This still seems like an overcomplex as per the comment on lines 160 + 161, a computation with ceil should be sufficient, but there's corner cases I haven't checked.

This comment has been minimized.

@gtrensch

gtrensch Apr 7, 2017
Collaborator

I agree with Alex. To convert milliseconds into tics to step boundary, something like this
ceil( ms * STEPS_PER_MS ) * TICS_PER_STEP
will do the same, I believe. Also preserving infinity is an issue here. But however, I think this goes beyond this PR and would be a refactoring task.

This comment has been minimized.

@apeyser

apeyser Apr 7, 2017
Contributor

@heplesser @jakobj @gtrensch Agreed with guido -- I suggested later on that we make another PR with the changes in Time that aren't absolutely connected with the caching in Events or inverting the div operations.

This comment has been minimized.

@heplesser

heplesser Apr 7, 2017
Contributor

Agreed.

void
calculate_steps()
{
if ( tics == LIM_POS_INF.tics )

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

For safety's sake tics >= ...

This comment has been minimized.

@apeyser

apeyser Apr 6, 2017
Contributor

Is that safer? I think an assert for tics <= ... should be there -- it should never be greater than the limit, that's just as broken as missing it altogether.

This comment has been minimized.

@heplesser

heplesser Apr 7, 2017
Contributor

I agree with @apeyser

{
steps_ = LIM_POS_INF.steps;
}
else if ( tics == LIM_NEG_INF.tics )

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

tics <= ...

@@ -537,6 +562,9 @@ class Time
{
return ld_round( ms * Range::STEPS_PER_MS );
}

private:
unsigned long steps_;

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

Time can be negative, so steps_ must be long, not unsigned long. This, unfortunately, makes it difficult to mark steps_ as invalid by assigning a special value.

@@ -309,6 +309,8 @@ class Event
*/
Time stamp_;

unsigned long steps_;

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017
Contributor

This is actually "doppeltgemoppelt": If we cache the steps value already in the Time object stamp_, we do not need to store it additionally in the Event object.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Apr 5, 2017

@apeyser @jakobj I think I understand now why we see the slight increase in build times: For each connection created, sender->send_test_event() creates an Event object, which creates a Time object stamp_. In the current version, this requires computing the step_ value of the Time object, which is never used in connectivity testing. Since this is done once for every single synapse, it hurts.

Furthermore, the current solution now caches the step count twice in each Event: once inside the Time stamp_ and once explicitly as steps_. This makes little sense.

The reason that we save time by caching during event delivery is that the spike time is set once and the event is then sent to all spike targets on the local process. I believe this is the only case in which the stamps value will be re-used many times and where caching thus makes sense.

Therefore, I would propose the following approach:

  • The Time class does not cache the steps value; it just replaces division by multiplication; this pays off, since the scaling factor needs to be recomputed only when the resolution changes.
  • The Event class caches the step value of the time stamp using lazy evaluation.
  • In the Event class, we can exploit the fact that events only occur for t > 0, so that we can use steps_ == 0 as invalid marker (or -1 to be even more on the safe side).
    This will avoid the unnecessary computation during the build phase and still ensure gains during the simulation phase.
@heplesser
Copy link
Contributor

@heplesser heplesser commented Apr 5, 2017

@jakobj Which benchmark did you use?

@heplesser heplesser removed the request for review from gtrensch Apr 5, 2017
@@ -162,7 +165,7 @@ Time::fromstamp( Time::ms_stamp t )
// intended ones.
tic_t n = static_cast< tic_t >( t.t * Range::TICS_PER_MS );
n -= ( n % Range::TICS_PER_STEP );
long s = n / Range::TICS_PER_STEP;
long s = n * Range::TICS_PER_STEP_INV;
double ms = s * Range::MS_PER_STEP;

This comment has been minimized.

@apeyser

apeyser Apr 6, 2017
Contributor

What we need here is just MS_PER_TIC (MS_PER_STEP * STEP_PER_TIC).

This still seems like an overcomplex as per the comment on lines 160 + 161, a computation with ceil should be sufficient, but there's corner cases I haven't checked.

@@ -54,6 +54,7 @@ const double Time::Range::TICS_PER_MS_DEFAULT = CONFIG_TICS_PER_MS;
const tic_t Time::Range::TICS_PER_STEP_DEFAULT = CONFIG_TICS_PER_STEP;

tic_t Time::Range::TICS_PER_STEP = Time::Range::TICS_PER_STEP_DEFAULT;
double Time::Range::TICS_PER_STEP_INV = 1. / static_cast< double >( Time::Range::TICS_PER_STEP );

This comment has been minimized.

@apeyser

apeyser Apr 6, 2017
Contributor

The cast is unnecessary since the op is 1. / int, it's a float result. But it's a matter of taste what is clearer.

@@ -77,7 +78,7 @@ Time::compute_max()
const tic_t tmax = std::numeric_limits< tic_t >::max();

tic_t tics;
if ( lmax < tmax / Range::TICS_PER_STEP ) // step size is limiting factor
if ( lmax < tmax * Range::TICS_PER_STEP_INV ) // step size is limiting factor

This comment has been minimized.

@apeyser

apeyser Apr 6, 2017
Contributor

And we probably don't need to bother with changing these, or inside limit, or inside set_resolution and so on... just in the code that gets deeply looped, which is Time constructors and operators on Time objects.

This comment has been minimized.

@heplesser

heplesser Apr 7, 2017
Contributor

@apeyser This does not matter to performance here, but isn't it a good idea to do calculations consistent all over? So I think multiplication instead of division here is fine.

This comment has been minimized.

@gtrensch

gtrensch Apr 7, 2017
Collaborator

@apeyser @heplesser I'd vote for readability. In struct range{} there are e.g. definitions for STEPS_PER_MS and MS_PER_STEP. Wouldn't it be consequent to have STEPS_PER_TIC ?

This comment has been minimized.

@apeyser

apeyser Apr 7, 2017
Contributor

@gtrensch Agreed -- in general the "INV" of "X_PER_Y" would be clearer as "Y_PER_X".

This comment has been minimized.

@apeyser

apeyser Apr 7, 2017
Contributor

@heplesser This is also a question of taste. On the one hand, not changing anything that doesn't need to be changed is "clearer" since we have a minimal change; on the other hand, using all * instead of / is also "clearer".

void
calculate_steps()
{
if ( tics == LIM_POS_INF.tics )

This comment has been minimized.

@apeyser

apeyser Apr 6, 2017
Contributor

Is that safer? I think an assert for tics <= ... should be there -- it should never be greater than the limit, that's just as broken as missing it altogether.

@jakobj
Copy link
Contributor Author

@jakobj jakobj commented Apr 7, 2017

@apeyser @heplesser thanks for all the helpful comments. I am now removing the caching in the Time objects and implement lazy evaluation in Event. Should I nevertheless implement your suggested changes in the Time class (like if ( tics == LIM_POS_INF.tics ) -> if ( tics >= LIM_POS_INF.tics ))?
For benchmarks I am using the hpc_benchmark.sli with 12500 neurons per node, 8 threads/process. I've added memory consumption for completeness (and because my benchmarking pipeline produces that panel anyway ;) ).

@apeyser
Copy link
Contributor

@apeyser apeyser commented Apr 7, 2017

@jakobj My tendency is to make a minimal change in a change set. Maybe open another pull request with just the fixes to the Time class class?

@jakobj jakobj force-pushed the jakobj:enh/avoid_divisions branch from d05335a to 7d0056d Apr 12, 2017
jakobj added a commit to jakobj/nest-simulator that referenced this pull request Apr 12, 2017
@jakobj
Copy link
Contributor Author

@jakobj jakobj commented Apr 12, 2017

I have now implemented the comments as far as I can see, and created a separate PR (#706) to implement all changes in the time class that are not directly related to this PR. Please have another look. I wasn't sure about the change of X_PER_Y_INV to Y_PER_X since I am fine with either.

@jakobj jakobj force-pushed the jakobj:enh/avoid_divisions branch from 4a2dde3 to 4c2bb11 Jun 12, 2017
@jakobj
Copy link
Contributor Author

@jakobj jakobj commented Jun 12, 2017

@heplesser @apeyser I just fixed the conflicts, so this is ready to be checked again.

jakobj added a commit to jakobj/nest-simulator that referenced this pull request Jun 12, 2017
@@ -66,7 +66,14 @@ STDPPLHomCommonProperties::set_status( const DictionaryDatum& d,
CommonSynapseProperties::set_status( d, cm );

updateValue< double >( d, names::tau_plus, tau_plus_ );
tau_plus_inv_ = 1. / tau_plus_;
if ( tau_plus_ > 0. )

This comment has been minimized.

@apeyser

apeyser Jun 12, 2017
Contributor

Is there a constraint on how small tau_plus is? If there wasn't before, then you don't need to add it, but it's probably worth documenting if there's a numeric instability here.

@@ -99,7 +103,7 @@ nest::Archiving_Node::get_K_value( double t )
if ( t > history_[ i ].t_ )
{
return ( history_[ i ].Kminus_
* std::exp( ( history_[ i ].t_ - t ) / tau_minus_ ) );
* std::exp( ( history_[ i ].t_ - t ) * tau_minus_inv_ ) );

This comment has been minimized.

@apeyser

apeyser Jun 12, 2017
Contributor

I'd be curious if there's a way to state this in a more numerically stable way -- just to keep in mind for the future

This comment has been minimized.

@heplesser

heplesser Jun 13, 2017
Contributor

@apeyser What kind of numerical stability problems do you see here?

This comment has been minimized.

@apeyser

apeyser Jun 13, 2017
Contributor

We'd have to run the numbers -- but we're combining root(tau, e^t1/e^t2), so it's worth looking at the relative scales of all three numbers (how big can t1, t2 and t3 get?) to see whether we can blow up in different ways. This formulation looks reasonable -- but I don't know the cases, so i can't say of the top of my head -- just asking whether it's been thought about.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 13, 2017

@jakobj It seems something may have gone wrong during the merge, since NEST does not build on Travis:

home/travis/build/nest/nest-simulator/nestkernel/nest_time.cpp: In function ‘std::ostream& operator<<(std::ostream&, const nest::Time&)’:
/home/travis/build/nest/nest-simulator/nestkernel/nest_time.cpp:220:1: error: expected ‘}’ at end of input
Copy link
Contributor

@heplesser heplesser left a comment

@jakobj One comment and one missing curly brace left.

@@ -178,7 +178,7 @@ class Event
*
* @see NEST Time Memo, Rule 3
*/
long get_rel_delivery_steps( const Time& t ) const;
long get_rel_delivery_steps( const Time& t );

This comment has been minimized.

@heplesser

heplesser Jun 13, 2017
Contributor

Could you add a note in the doxygen comment for the method why it is not const? It will make it easier to understand the code, given that developers usually assume that getters are const.

Alternatively, one could keep the method const and declare stamp_steps_ a mutable. @apeyser, do you have an opinion on which approach would make for more maintainable code in the long run?

Since @apeyser favors mutable (as I do), I suggest we go for const method with mutable stamp_steps_.

@@ -211,7 +215,6 @@ std::ostream& operator<<( std::ostream& strm, const Time& t )
strm << t.get_ms() << " ms (= " << t.get_tics()
<< " tics = " << t.get_steps()
<< ( t.get_steps() != 1 ? " steps)" : " step)" );
}

This comment has been minimized.

@heplesser

heplesser Jun 13, 2017
Contributor

@jakobj This closing brace is required.

This comment has been minimized.

@apeyser

apeyser Jun 13, 2017
Contributor

I'd go with mutable -- it's precisely for this case of cached values.

@jakobj jakobj force-pushed the jakobj:enh/avoid_divisions branch from 4c2bb11 to ffeddbe Jun 13, 2017
@jakobj
Copy link
Contributor Author

@jakobj jakobj commented Jun 13, 2017

Thanks for the input. Done implementing your proposed changes. Didn't know about mutable before - nice feature for this type of situation.

jakobj added a commit to jakobj/nest-simulator that referenced this pull request Jun 13, 2017
@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 13, 2017

@jakobj It seems clang-format is unhappy.

Copy link
Contributor

@heplesser heplesser left a comment

I approve, now only Travis/clang-format needs to become happy as well.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 14, 2017

Release notes: Performance improvements through more efficient numerics in Time

@heplesser heplesser merged commit 18bb2cc into nest:master Jun 14, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jakobj added a commit to jakobj/nest-simulator that referenced this pull request Jun 20, 2017
apeyser added a commit that referenced this pull request Jun 29, 2017
Minor refactoring in Time as discussed in PR #685
aserenko added a commit to aserenko/nest-simulator that referenced this pull request May 15, 2018
Make the variable nearest_neighbor_K_value
conformant to PR nest#685:
multiply by tau_minus_inv_
instead of dividing by tau_minus_.
@jakobj jakobj deleted the jakobj:enh/avoid_divisions branch Sep 13, 2018
@jakobj jakobj restored the jakobj:enh/avoid_divisions branch Sep 13, 2018
@jakobj jakobj deleted the jakobj:enh/avoid_divisions branch Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.