[c++] Fix heap management in fastercsx

[johnkerl]

Issue and/or context: Found while debugging (details below).

Issue: #3345; see also #3304.

Changes:

We have a pair of macros tdb_new_array and tdb_delete_array. As written, these are pairing malloc with delete []. This is incorrect behavior, which this PR fixes.

Also note that this defect is new to our modified vendoring of threadpool/status.h:

• Source:
  • https://github.com/TileDB-Inc/TileDB/blob/a34830c2b9fe4c00526e497630f251263b5e468b/tiledb/common/heap_memory.h#L165-L168
  • https://github.com/TileDB-Inc/TileDB/blob/a34830c2b9fe4c00526e497630f251263b5e468b/tiledb/common/heap_memory.h#L96-L113
  • https://github.com/TileDB-Inc/TileDB/blob/a34830c2b9fe4c00526e497630f251263b5e468b/tiledb/common/heap_memory.h#L115-L121
• Modified vendoring:

  • TileDB-SOMA/libtiledbsoma/src/external/include/thread_pool/status.h

    Lines 60 to 64 in ca00d5b

    	#define tdb_delete_array(p) \
    	if (p) { \
    	delete[] p; \
    	}
    	#define tdb_new_array(T, size) (T*)malloc(size * sizeof(T))

Given the narrative at #3304 (comment) I did not review these files on my review of #3304; I'll take another look now including a diff against the core vendoring-source files.

Also note that libtiledbsoma/src/external/include/thread_pool/status.h is not new to #3304, but newly exercised by #3304.

Notes for Reviewer:

I found this while debugging an issue: on my laptop (but not in CI) there is a segfault here:

TileDB-SOMA/apis/python/tests/test_fastercsx.py

Lines 352 to 363 in ca00d5b

	def test_bad_shapes(context: soma.SOMATileDBContext, rng: np.random.Generator) -> None:
	"""Test bad/mismatched shape."""
	sp = sparse.identity(32, dtype=np.int8).tocoo()
	for shp in [
	(sp.shape[0] - 1, sp.shape[1]),
	(sp.shape[0], sp.shape[1] - 1),
	]:
	with pytest.raises(IndexError):
	# this will also log an error - which is expected behavior for parallel_for/thread_pool
	fastercsx.CompressedMatrix.from_ijd(
	sp.row, sp.col, sp.data, shp, "csr", True, context
	)

This intends to catch an exception thrown through this callchain, as revealed by source-level debugging this morning:

• TileDB-SOMA/apis/python/src/tiledbsoma/_fastercsx.py

  Lines 86 to 88 in ca00d5b

  	compress_coo(
  	context.native_context, (n_major, n_minor), i, j, d, indptr, indices, data
  	)

• TileDB-SOMA/libtiledbsoma/src/utils/fastercsx.h

  Lines 310 to 320 in ca00d5b

  	template <class VALUE, class COO_IDX, class CSX_MINOR_IDX, class CSX_MAJOR_IDX>
  	void compress_coo(
  	ThreadPool* const tp,
  	const Shape& shape,
  	uint64_t nnz,
  	const std::vector<std::span<const COO_IDX>>& Ai,
  	const std::vector<std::span<const COO_IDX>>& Aj,
  	const std::vector<std::span<const VALUE>>& Ad,
  	std::span<CSX_MAJOR_IDX> Bp,
  	std::span<CSX_MINOR_IDX> Bj,
  	std::span<VALUE> Bd) {

• TileDB-SOMA/libtiledbsoma/src/utils/fastercsx.h

  Lines 389 to 398 in ca00d5b

  	compress_coo_inner_right_(
  	row_partition,
  	partition_bits,
  	n_col,
  	Ai_,
  	Aj_,
  	Ad_,
  	Bp_right_span,
  	Bj,
  	Bd);

• TileDB-SOMA/libtiledbsoma/src/utils/fastercsx.h

  Line 286 in ca00d5b

  throw std::out_of_range("Coordinate out of range.");

Here is a standalone repro which crashes for me, taken from apis/python/tests/test_fastercsx.py::test_bad_shapes:

#!/usr/bin/env python

import tiledbsoma
import tiledbsoma._fastercsx as fastercsx

import scipy.sparse
import numpy as np

context = tiledbsoma.SOMATileDBContext()

sp = scipy.sparse.identity(32, dtype=np.int8).tocoo()
for shp in [
    (sp.shape[0] - 1, sp.shape[1]),
    (sp.shape[0], sp.shape[1] - 1),
]:
    try:
        fastercsx.CompressedMatrix.from_ijd(
            sp.row, sp.col, sp.data, shp, "csr", True, context,
        )
    except IndexError:
        print("Caught IndexError as expected")

This does crash for me on my laptop; I does not crash for me on an Ubuntu system, and our multi-platform CI is green on main. Nonetheless I ran valgrind on an Ubuntu system, to investigate. I found the following:

• Before: https://gist.github.com/johnkerl/59c71f2928e6fdc6b991a19beed47e93 (double-spaced for convenience)
• After applying this patch: https://gist.github.com/johnkerl/71755befa2d8e82be151b4140668a89a (double-spaced for convenience)

The valgrind output

apis/python/tests/test_fastercsx.py ==9085== Mismatched free() / delete / delete []

led me directly to the codemod addressed on this PR.

Do note however that even after a clean and rebuild I do still experience the segfault with the above scenario on my laptop. This means that I've found a heap-corruption issue on this PR, but there appears to be something else going on as well.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.74%. Comparing base (ca00d5b) to head (c74ca98).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3365      +/-   ##
==========================================
+ Coverage   85.64%   85.74%   +0.09%     
==========================================
  Files          57       57              
  Lines        6201     6201              
==========================================
+ Hits         5311     5317       +6     
+ Misses        890      884       -6     

Flag	Coverage Δ
python	85.74% <ø> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
python_api	85.74% <ø> (+0.09%)	⬆️
libtiledbsoma	∅ <ø> (∅)

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[johnkerl]

Given the narrative at #3304 (comment) I did not review these files on my review of #3304; I'll take another look now including a diff against the core vendoring-source files.

I ran the following (both my core and my tiledbsoma checkouts having had fresh pulls on dev and main respectively):

diff -w \
  ~/git/TileDB-Inc/TileDB/tiledb/sm/misc/parallel_functions.h \
  ~/git/single-cell-data/TileDB-SOMA/libtiledbsoma/src/utils/parallel_functions.h

diff -w \
  ~/git/TileDB-Inc/TileDB/tiledb/common/thread_pool/producer_consumer_queue.h \
  ~/git/single-cell-data/TileDB-SOMA/libtiledbsoma/src/external/include/thread_pool/producer_consumer_queue.h

diff -w \
  ~/git/TileDB-Inc/TileDB/tiledb/common/thread_pool/thread_pool.h \
  ~/git/single-cell-data/TileDB-SOMA/libtiledbsoma/src/external/include/thread_pool/thread_pool.h

diff -w \
  ~/git/TileDB-Inc/TileDB/tiledb/common/thread_pool/thread_pool.cc \
  ~/git/single-cell-data/TileDB-SOMA/libtiledbsoma/src/external/src/thread_pool/thread_pool.cc

with the following output: https://gist.github.com/johnkerl/ebb915ce74e424bc57a4002983792209

Looks to me like:

• The bug fixed here
• Whitespace/clang-format
• Some namespacing changes, e.g. using optional and optional<std::exception_ptr> -> std::optional<std::exception_ptr>
• namespace tiledb::sm -> namespace tiledbsoma
• std::lock_guard lock{mutex_}; -> std::scoped_lock lock{mutex_};
• Etc.
• Removal of one unused/unneeded function

Looks good to me -- CC @nguyenv if you would like to take a peek as a second pair of eyes.

[bkmartinjr]

LGTM. Thanks John.