#4517: DynamicView: Fix deallocation extent

[lifflander]

Fixes #4517

[lifflander]

This change fixes the performance issues that were observed in EMPIRE

[PhilMiller]

Was the code previously just deallocating some huge number of garbage entries that it shouldn't have been?

[rppawlo]

Thanks @lifflander for finding this! We have verified that this fixes the empire issues.

@ndellingwood - when this merges into kokkos, can you snapshot into trilinos. This shoudl get the empire sync of trilinos going again!

[lifflander]

Was the code previously just deallocating some huge number of garbage entries that it shouldn't have been?

Yes, they are nullptr... so just wasting time. Before I changed the code, the deallocator was going over all the entries also but it tested for nullptr before actually calling the deallocator.

[masterleinad]

Looks like this makes the unit tests segfault.

[dalg24]

This feels so sketchy. Why is it not something like

Suggested change

	auto const len = *reinterpret_cast<size_t*>(m_chunks + m_chunk_max + 1);
	size_t const len = *(m_chunks + m_chunk_max + 1);

Also what was wrong with unsigned?

[lifflander]

m_chunks is a pointer type that is abused to store a numerical value.

[lifflander]

kokkos/containers/src/Kokkos_DynamicView.hpp

Lines 356 to 360 in a1d045d

	size_t size() const noexcept {
	size_t extent_0 =
	*reinterpret_cast<const size_t*>(m_chunks_host + m_chunk_max + 1);
	return extent_0;
	}

Something's clearly not right/working here

[lifflander]

I accidentally picked the wrong entry for deallocation:

• m_chunks + m_chunk_max -> number of chunks used
• m_chunks + m_chunk_max + 1 -> extent (number of chunks * chunk_size)

[PhilMiller]

The CI failure from Jenkins is spurious relative to this PR -

1: [ RUN      ] openmp.reducers_half_t
1: /var/jenkins/workspace/Kokkos/core/unit_test/TestReducers.hpp:398: Failure
1: Expected equality of these values:
1:   prod_scalar
1:     Which is: 13120.000000
1:   reference_prod
1:     Which is: 13128.000000
1: [  FAILED  ] openmp.reducers_half_t (0 ms)

It probably represents an independent issue to be investigated, though.

https://cloud.cees.ornl.gov/jenkins-ci/blue/rest/organizations/jenkins/pipelines/Kokkos/runs/7162/nodes/47/steps/207/log/?start=0

[masterleinad]

Suggested change

	size_t const len = *reinterpret_cast<size_t*>(m_chunks + m_chunk_max);
	uintptr_t const len = *reinterpret_cast<uintptr_t*>(m_chunks + m_chunk_max);

just to be conforming with access in other locations. Also, I would very much prefer if the allocation counters are stored in separate variables (inside ChunkedArrayManager). The need to reinterpret_cast here feels pretty sketchy and is confusing regarding readability.

[lifflander]

Ok, I've made the change to uintptr_t

[lifflander]

I agree that allocating them separately would be increase code readability, but this PR is not making this worse (i.e., DynamicView has been this way since the beginning). Due to how critical this is for EMPIRE right now, I would like to get this moved forward as is and then push for refactoring later to improve code readability.

[PhilMiller]

Retest this please

[ndellingwood]

Cherry-picked to 3.5 with #4538