Skip to content

Optimize code generated for diam and area.#2914

Merged
pramodk merged 7 commits into
masterfrom
1uc/optimize-diam-area
Jun 21, 2024
Merged

Optimize code generated for diam and area.#2914
pramodk merged 7 commits into
masterfrom
1uc/optimize-diam-area

Conversation

@1uc

@1uc 1uc commented Jun 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

azure-pipelines Bot commented Jun 13, 2024

Copy link
Copy Markdown

✔️ 2d75d7e -> Azure artifacts URL

@codecov

codecov Bot commented Jun 13, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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 
@@           Coverage Diff           @@
##           master    #2914   +/-   ##
=======================================
  Coverage   67.22%   67.22%           
=======================================
  Files         569      569           
  Lines      104691   104702   +11     
=======================================
+ Hits        70378    70388   +10     
- Misses      34313    34314    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot

This comment has been minimized.

Sign in to view
@1uc 1uc marked this pull request as ready for review June 13, 2024 16:38
@1uc

1uc commented Jun 13, 2024

Copy link
Copy Markdown
Collaborator Author

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

@pramodk

pramodk commented Jun 14, 2024

Copy link
Copy Markdown
Member

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 nrnhines/266806@b3815a0 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 nrnhines/266806@b3815a0).

pramodk
pramodk approved these changes Jun 14, 2024
View reviewed changes

@pramodk pramodk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from the performance comparison aspects.

Comment thread src/neuron/cache/mechanism_range.hpp
@1uc

1uc commented Jun 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

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

azure-pipelines Bot commented Jun 14, 2024

Copy link
Copy Markdown

✔️ 7cb0aff -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@1uc

1uc commented Jun 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@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.

@1uc 1uc force-pushed the 1uc/optimize-diam-area branch from 7cb0aff to 1e41a48 Compare June 17, 2024 07:21
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@1uc 1uc mentioned this pull request Jun 17, 2024
Merged
@azure-pipelines

azure-pipelines Bot commented Jun 17, 2024

Copy link
Copy Markdown

✔️ a350cee -> Azure artifacts URL

@pramodk

pramodk commented Jun 17, 2024

Copy link
Copy Markdown
Member

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

@1uc 1uc mentioned this pull request Jun 18, 2024
Merged
@sonarqubecloud

sonarqubecloud Bot commented Jun 21, 2024

Copy link
Copy Markdown

Quality Gate Passed 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

azure-pipelines Bot commented Jun 21, 2024

Copy link
Copy Markdown

✔️ 0b45c4d -> Azure artifacts URL

nrnhines
nrnhines approved these changes Jun 21, 2024
View reviewed changes

@nrnhines nrnhines left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job. I've verified that this clearly fixes the performance issue with diam.
I tested with the sodium longitudinal diffusion example. Results

Init&Run time     version
1.9                      8.2.4
5.2.                     master
2.9                      thisPR
2.9                      thisPR with diam replaced by diamr range variable in KINETIC

The model files come from nrn/share/examples/nrniv/nmodl and the relevant files are

nacur.mod
ionleak.mod
nadifl.mod
nadifl2.hoc
nadifl2.ses

launch with

nrnivmodl
nrngui nadifl2.hoc

press the Init&Run button and note the RealTime at the bottom of the RunControl

@bbpbuildbot

bbpbuildbot commented Jun 21, 2024

Copy link
Copy Markdown
Collaborator

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

Status and direct links:

@pramodk

pramodk commented Jun 21, 2024

Copy link
Copy Markdown
Member

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.

@pramodk pramodk merged commit 61b9ca2 into master Jun 21, 2024
@pramodk pramodk deleted the 1uc/optimize-diam-area branch June 21, 2024 13:11
@1uc

1uc commented Jun 21, 2024

Copy link
Copy Markdown
Collaborator Author

See #2787.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pramodk pramodk pramodk approved these changes
@nrnhines nrnhines nrnhines approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@1uc @bbpbuildbot @pramodk @nrnhines