Fix consensus_tree segfault on arm64 by implementing dynamic stack/heap allocation #243
+248
−133
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add stack threshold (8192) and heap limit (100000) constants - Create DynamicBitset class for Xswitch with stack/heap allocation - Template consensus_tree_impl to handle both array and vector S - Update error messages with specific limits - Update tests for new limits and add heap allocation tests Co-authored-by: ms609 <1695515+ms609@users.noreply.github.com>
Co-authored-by: ms609 <1695515+ms609@users.noreply.github.com>
Co-authored-by: ms609 <1695515+ms609@users.noreply.github.com>
Co-authored-by: ms609 <1695515+ms609@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor consensus_tree logic for dynamic tree sizes
Fix consensus_tree segfault on arm64 by implementing dynamic stack/heap allocation
Feb 4, 2026
Performance benchmark results
|
Performance benchmark results
|
Performance benchmark results
|
Owner
|
@copilot Good start thank you; I had to make some improvements to get this going. Now I'm seeing: Here's the test that's triggering it. test_that("ClusterTable with complex trees", {
skip_if_not_installed("TreeDist", "2.9.2.9000")
library("TreeDist")
# Test exposes failures in C++ - constexpr not playing nicely with Rcpp
# Specifically if replacing
# const int16 L_COL = int16(0);
# const int16 R_COL = int16(1);
# const int16 X_COLS = int16(2);
# hard-coding, using enum all fails.
tr1 <- structure(list(
edge = structure(c(8L, 8L, 9L, 10L, 10L, 9L, 11L, 11L, 8L, 12L, 12L,
1L, 9L, 10L, 2L, 3L, 11L, 4L, 5L, 12L, 6L, 7L),
dim = c(11L, 2L)),
Nnode = 5L, tip.label = c("t1", "t2", "t3", "t4", "t5", "t6", "t7")),
class = "phylo", order = "preorder")
tr2 <- structure(list(
edge = structure(c(8L, 9L, 10L, 10L, 9L, 11L, 11L, 8L, 12L, 12L, 8L,
9L, 10L, 1L, 2L, 11L, 3L, 4L, 12L, 5L, 6L, 7L),
dim = c(11L, 2L)),
Nnode = 5L, tip.label = c("t1", "t2", "t3", "t4", "t5", "t6", "t7")),
class = "phylo", order = "preorder")
t4 <- list(a = tr1, b = tr2, c = tr1, d = tr2)
r4 <- RootTree(t4, 1)
expect_equal(as.numeric(RobinsonFoulds(r4)), c(8, 0, 8, 8, 0, 8))
}) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The consensus_tree function segfaulted on arm64 for trees exceeding 16383 leaves due to fixed-size stack allocations (
std::bitset<16384>andstd::array<int32, 65532>). The 33333-leaf test case triggered stack overflow.Changes
Dynamic allocation strategy:
ClusterTable.h:
DynamicBitsetclass that switches betweenstd::bitset<8193>(stack) andstd::vector<bool>(heap) based on tree sizestd::bitset<ct_max_leaves + 1> XswitchwithDynamicBitset XswitchXswitch.resize(n_leaves + 1)consensus.cpp:
consensus_tree_impl<StackContainer>std::array<int32, 32768>for small trees,std::vector<int32>for largeCT_PUSH/CT_POPwork transparently with both containersTests:
Backward compatibility
The
ct_max_leavesconstant (16383) is preserved for TreeDist 2.9.2 compatibility, though no longer enforced as a hard limit.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.