Optimize code generated for `diam` and `area`.

1uc commented 3 weeks ago

Optimizes nocmodl generated code to use the mechanism cache when accessing diam and area as described in #2913.

For performance testing I use:

NEURON {
  SUFFIX two_radii
  USEION ca READ cai
  NONSPECIFIC_CURRENT il
  RANGE y, inv
}

ASSIGNED {
  v
  y
  il
  inv
  diam
  area
  cai
}

INITIAL {
  y = 1.0*diam + area + cai
  inv = 1.0 / square_diam()
}

BREAKPOINT {
  il = square_diam() / inv * 0.001 * (v - 20.0)
}

FUNCTION square_diam() {
  square_diam = diam*diam + area
}

The name two_radii is to be able to grep for diam, we added the ion ca to compare with how it's solved for ions. The logic is to use inv to create a very expensive multiplication with 1.0.

The python file to measure the performance is:

import time

import numpy as np
from neuron import gui
from neuron import h

import matplotlib.pyplot as plt

nsec = 100
nseg = 1000

sections = [h.Section() for _ in range(nsec)]

for s in sections:
    s.nseg = nseg
    s.insert("two_radii")

t_hoc = h.Vector().record(h._ref_t)
v_hoc = h.Vector().record(sections[0](0.5)._ref_v)

h.stdinit()

t0 = time.time();
h.continuerun(50.0)
elapsed = time.time() - t0

print(f"{elapsed=}")

t = np.array(t_hoc.as_numpy())
v = np.array(v_hoc.as_numpy())

# plt.plot(t, v)
# plt.show()

Edit: I realized I was measuring with Debug. After switching to Release I needed to refine the measurement slightly (it now scales linearly with tstop). The time after fix is 5.6s and before 25s.

azure-pipelines[bot] commented 3 weeks ago

✔️ 2d75d7e1027e47ca736471761bfc05f1c0965fa8 -> Azure artifacts URL

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.22%. Comparing base (8cb9ec8) to head (a350cee). Report is 4 commits behind head on master.

:exclamation: Current head a350cee differs from pull request most recent head 0b45c4d

Please upload reports for the commit 0b45c4d to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #2914 +/- ## ======================================= Coverage 67.22% 67.22% ======================================= Files 569 569 Lines 104691 104702 +11 ======================================= + Hits 70378 70388 +10 - Misses 34313 34314 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bbpbuildbot commented 3 weeks ago

Logfiles from GitLab pipeline #216817 (:white_check_mark:) have been uploaded here!

Status and direct links:

1uc commented 3 weeks ago

@nrnhines @pramodk could you please check if it at least partly solves the performance issue?

pramodk commented 3 weeks ago

Here are the numbers:

I am using https://github.com/nrnhines/266806/ and with single thread . I am using Morphology_1/mod_files/cdp5StCmod.mod without https://github.com/nrnhines/266806/commit/b3815a0b3df60bf6d9a5aadb843a4e900c35748f i.e. use diam directly :

## Master Branch
NEURON RUN took  30.94808864593506

   Path                   Min time/rank Max time/rank Avg time/rank Time %
      state-cdp5StCmod     18.409768     18.409768     18.409768   59.448159

## This PR

NEURON RUN took  21.428975582122803

   Path                   Min time/rank Max time/rank Avg time/rank Time %
      state-cdp5StCmod      9.139341      9.139341      9.139341   42.610451

And this time is similar to the one if we cache diam as a RANGE variable (as done in https://github.com/nrnhines/266806/commit/b3815a0b3df60bf6d9a5aadb843a4e900c35748f).

1uc commented 3 weeks ago

While implementing the same solution in NMODL, I noticed, that calling function from Python leads to a SEGFAULT. I'm debugging, but more changes are needed for this PR.

This has now been fixed, essentially the m_offset for MechanismInstance was incorrect for dptr_field.

azure-pipelines[bot] commented 3 weeks ago

✔️ 7cb0affc9b9ae614e89069feb79be070298c25e4 -> Azure artifacts URL

bbpbuildbot commented 3 weeks ago

Logfiles from GitLab pipeline #216999 (:white_check_mark:) have been uploaded here!

Status and direct links:

1uc commented 2 weeks ago

@pramodk It now contains a fix for the SEGFAULT, I think it's principled, but it could use re-review. Sorry, CI seems to have gotten stuck, I'll rerun.

bbpbuildbot commented 2 weeks ago

Logfiles from GitLab pipeline #217187 (:white_check_mark:) have been uploaded here!

Status and direct links:

bbpbuildbot commented 2 weeks ago

Logfiles from GitLab pipeline #217276 (:white_check_mark:) have been uploaded here!

Status and direct links:

azure-pipelines[bot] commented 2 weeks ago

✔️ a350ceec411f51b28c489e753ea232b44f2d4277 -> Azure artifacts URL

pramodk commented 2 weeks ago

@nrnhines : I have tested this and confirmed diam related perf regression is addressed. Do you to take a quick look at the change?

sonarcloud[bot] commented 2 weeks ago

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

azure-pipelines[bot] commented 2 weeks ago

✔️ 0b45c4dcf3cf695145f6970a3bf2b43399241d84 -> Azure artifacts URL

bbpbuildbot commented 2 weeks ago

Logfiles from GitLab pipeline #218223 (:no_entry:) have been uploaded here!

Status and direct links:

pramodk commented 2 weeks ago

I have neurodamus people about Gitlab failures. As this CI was green and failures are completely unrelated, I think we should skip/ignore gitlab CI here.

1uc commented 2 weeks ago

See #2787.

neuronsimulator / nrn

Optimize code generated for `diam` and `area`. #2914

Codecov Report

Quality Gate passed