Perf: remove O(n) from special_map when constructing a list when we already know the element type

[jacinta-stacks]

Related to https://github.com/stx-labs/core-epics/issues/117

special_map previously called Value::cons_list(mapped_results, epoch) to build its output list. That function internally calls construct_parent_list_type, which walks every element calling type_of and folding with least_supertype. This is an O(n) second pass over results that had already been computed during the mapping loop itself. So now it accumulates the element type incrementally in the loop (element_type_opt) and calls Value::cons_list_typed, which goes straight to ListTypeData::new_list. The O(n) post-pass is eliminated entirely.

Benchmark Results cargo bench -p clarity --bench sequence_ops -- special_map, compared against pre-change baseline

Benchmark	Before	After	Change
unary_int/100	32.09 µs	31.44 µs	−1.9%
binary_int/100	49.98 µs	49.41 µs	−1.4%
unary_int/1000	270.41 µs	263.39 µs	−3.1%
binary_int/1000	452.14 µs	446.43 µs	−2.0%
unary_int/8000	2.127 ms	2.043 ms	−3.9%
binary_int/8000	3.568 ms	3.455 ms	−3.2%
binary_int_unequal/1000vs100	138.84 µs	136.60 µs	−3.0%
binary_int_unequal/8000vs500	953.05 µs	940.05 µs	within noise

TLDR: Not very noticeable improvement...The improvement makes all (map ...) calls in every Clarity contract pay less per invocation, with no tradeoffs. Not sure how much that will actually affect block production but its a net improvement regardless.

[francesco-stacks]

lgtm! Just a couple of nits

[francesco-stacks]

unit tests are currently failing, otherwise lgtm

[benjamin-stacks]

My concern is that there is indeed a small trade-off here: We're gaining a bit of performance in exchange for a slight reduction in code maintainability because there are now two separate implementations of the same functionality.

I'm also not sure this is truly an O(n) reduction, because the way I understand it, we're replacing this:

for x in list:
    compute_value(x)
for x in list:
    compute_least_supertype(x, prev)

with this:

for x in list:
    compute_value(x)
    compute_least_supertype(x, prev)

so unless I'm misunderstanding something (which is very possible!), the performance improvement is only from removing the overhead of the second iteration.

Thoughts?

[benjamin-stacks]

construct_parent_list_type() explicitly calls least_supertype_v2_1 instead.

I lack the context to judge whether that is currently okay (and it seems to me that your version is more correct), but even if it is: If there is ever a least_supertype_v_42_0, this might create a subtle divergence in behavior between cons_list and cons_list_typed.

[federico-stacks]

This worries me too.

At the moment, we rely on least_supertype_v2_1 under the hood, and changing this behavior could introduce regressions in block validation, replay, or genesis sync when processing epoch 2.0 blocks.

Another point to consider is whether we need to adjust the behavior of construct_parent_list_type(), since TypeSignature::parent_list_type() depends on least_supertype_v2_1.

As a side note, TypeSignature::parent_list_type() appears to be used directly in the type checker for version v2_05, while in least_supertype the same epoch should work with least_supertype_v2_0

[benjamin-stacks]

Fun fact, the type checker doesn't actually allow this, despite it being totally valid.

[federico-stacks]

My concern is that there is indeed a small trade-off here: We're gaining a bit of performance in exchange for a slight reduction in code maintainability because there are now two separate implementations of the same functionality.

I'm also not sure this is truly an O(n) reduction, because the way I understand it, we're replacing this:

for x in list:
    compute_value(x)
for x in list:
    compute_least_supertype(x, prev)

with this:

for x in list:
    compute_value(x)
    compute_least_supertype(x, prev)

so unless I'm misunderstanding something (which is very possible!), the performance improvement is only from removing the overhead of the second iteration.

Thoughts?

Correct. The save derive from removing an interation over N.
In the language of this PR, it moves from O(2n) to O(n) (basically for large numbers it was already O(n)).

[federico-stacks]

To properly evaluate the performance impact of this PR, it first needs to be rebased on top of:

• #7022, which introduces apply_evaluated and removes atom usage
• #7068, which fixes the off-by-one indexing issue and simplifies the special_map implementation

After those changes, what would remain from this PR is primarily the cons_list_typed approach.

I followed these steps locally and can confirm that the small performance improvements still hold, due to the removal of an iteration over N elements.

That said, I also have concerns about the use of least_supertype (instead of the current least_supertype_v2_1). Additionally, this would result in cons_list and cons_list_typed behaving differently (see discussion here: #6951 (review)).

We may also want to better understand the implications of using least_supertype_v2_1 in parent_list_type(), even in the current implementation (see #6951 (comment)).

[benjamin-stacks]

I vote we close this one. Given the concern from @federico-stacks and myself, and the only tiny perf improvement, I think this is not worth it.

[codecov]

Codecov Report

❌ Patch coverage is 92.42424% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.98%. Comparing base (faa4b28) to head (e3a50bf).
⚠️ Report is 456 commits behind head on develop.

Files with missing lines	Patch %	Lines
clarity/src/vm/functions/sequences.rs	87.50%	5 Missing ⚠️

❌ Your project status has failed because the head coverage (84.98%) is below the adjusted base coverage (84.99%). You can increase the head coverage or adjust the Removed Code Behavior.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #6951      +/-   ##
===========================================
- Coverage    85.00%   84.98%   -0.02%     
===========================================
  Files          412      412              
  Lines       219942   219975      +33     
  Branches       338      338              
===========================================
- Hits        186951   186941      -10     
- Misses       32991    33034      +43     

Files with missing lines	Coverage Δ
clarity-types/src/types/mod.rs	93.60% <100.00%> (+0.15%)	⬆️
clarity/src/vm/functions/sequences.rs	89.10% <87.50%> (-0.06%)	⬇️

... and 40 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update faa4b28...e3a50bf. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.