Remove non_owning_*::get_raw_pointer().

[1uc]

No description provided.

[1uc]

I would like towards allowing the stable identifiers to be reference counted. One prerequisite for this to work without risking segfaults is that nobody has the address of the stable identifier. Instead they should just have a non_owning_identifier of some sort (with or without container).

[azure-pipelines]

✔️ b92adf5 -> Azure artifacts URL

[codecov]

Codecov Report

Merging #2497 (159a007) into master (93c671b) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2497      +/-   ##
==========================================
- Coverage   61.49%   61.49%   -0.01%     
==========================================
  Files         624      624              
  Lines      119189   119190       +1     
==========================================
- Hits        73294    73292       -2     
- Misses      45895    45898       +3     

Files Changed	Coverage Δ
src/neuron/container/non_owning_soa_identifier.hpp	100.00% <ø> (ø)

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[1uc]

ModelDB CI passed, see:
https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/6173216548

Given how recent the addition of the method is I suspect that we can remove it without any breakage. I feels it crucial that we do not expose the address of the stable identifier. The reason for this it's a requirement for understanding the lifetime of the stable identifiers.

As mentioned before it's a requirement to make the stable identifiers reference counted; because if we expose the address, we can't know when it's safe to deallocate.

Finally, we can always add it back, but removing this method once it's in use could easily become a very hard problem.

[nrnhines]

I presume this has nothing to do with handles to raw pointers?

[1uc]

Summary: no, this function returns the address of the stable identifier. Not the raw pointer that can be stored in a data_handle.

Detailed explanation: There's two levels of lifetime at play. One is to keep rows in soa containers alive. This happens via owning_{handle,identifier} classes. Then there's the data_handle object, they loosely act as pointer to rows, but don't keep the row alive. They have access to a stable identifier, the size_t * inside the non_owning_identifier_without_container. The handles can check that they're valid by dereferencing that size_t *; if they get size_t(-1) then the memory that this handle points to has been freed.

You'll notice the second level of ownership, how does the data_handle know that it's safe to dereference the size_t *? Currently, it knows because we promise to leak the stable identifier, i.e. we simply never deallocate the size_t.

In simulations at BBP we've seen that the amount of memory used by these identifiers isn't high, but high enough to optimize it away (if possible).

The question is then: When is it safe to deallocate the stable identifier? Currently the answer is that we can't know, because someone could have gotten the address of the stable identifier via get_raw_pointer and then shared it with anyone. If we remove this method, then a possible answer is, that we could use reference counting to know when the last object that has access to this stable identifier has been deallocated. Once that happens, we are free to deallocate the stable identifier, because nobody is using it anymore. Essentially, the exact same logic as with std::shared_ptr.

However, this method should be removed regardless of the reference counting issue; simply because it makes the developers life much harder if it's we start using it.

[azure-pipelines]

✔️ 159a007 -> Azure artifacts URL

[bbpbuildbot]

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

Status and direct links:

• ✅ mac_m1_cmake_build: [cmake, ON, OFF, OFF, address]
• ✅ spack_setup
• ✅ build:nmodl
• ✅ build:neuron:nmodl:intel:legacy
• ✅ build:neuron:nmodl:intel:shared
• ✅ build:neuron:nmodl:nvhpc:acc:legacy
• ✅ build:neuron:nmodl:nvhpc:acc:shared
• ✅ build:neuron:nmodl:nvhpc:omp:legacy
• ✅ build:neuron:nmodl:nvhpc:omp
• ✅ test:neuron:nmodl:intel:legacy
• ✅ test:neuron:nmodl:intel:shared
• ✅ test:neuron:nmodl:nvhpc:acc:legacy
• ✅ test:neuron:nmodl:nvhpc:acc:shared
• ✅ test:neuron:nmodl:nvhpc:omp:legacy
• ✅ test:neuron:nmodl:nvhpc:omp