Skip to content

Conversation

@SyMind
Copy link
Member

@SyMind SyMind commented Nov 6, 2025

This PR refactors the StreamChunks trait architecture by introducing a new Chunks trait and splitting streaming logic from source implementations. The main changes include:

  • Introducing a new Chunks trait with a stream method that takes object pool, options, and callbacks
  • Converting StreamChunks::stream_chunks to return Box<dyn Chunks + 'a> instead of directly streaming
  • Removing the rope() method from the Source trait
  • Changing BoxSource from Arc<dyn Source> to Box<dyn Source>
Copilot AI review requested due to automatic review settings November 6, 2025 06:05
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the StreamChunks trait architecture by introducing a new Chunks trait and splitting streaming logic from source implementations. The main changes include:

  • Introducing a new Chunks trait with a stream method that takes object pool, options, and callbacks
  • Converting StreamChunks::stream_chunks to return Box<dyn Chunks + 'a> instead of directly streaming
  • Removing the rope() method from the Source trait
  • Changing BoxSource from Arc<dyn Source> to Box<dyn Source>

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/compat_source.rs Commented out entire test file
src/source_map_source.rs Updated to use new Chunks trait pattern with SourceMapSourceChunks wrapper
src/source.rs Removed rope() method, changed BoxSource from Arc to Box, updated StreamChunks trait signature
src/replace_source.rs Introduced ReplaceSourceChunks wrapper, removed rope() implementation, contains typo "chunsk"
src/raw_source.rs Added RawStringChunks and RawBufferSourceChunks wrappers using new Chunks trait
src/original_source.rs Added OriginalSourceChunks wrapper, updated to use new Chunks trait
src/lib.rs Exported new Chunks trait in stream_chunks module
src/helpers.rs Introduced Chunks trait, updated get_map and stream_and_get_source_and_map signatures
src/concat_source.rs Added ConcatSourceChunks wrapper, removed rope() implementation
src/cached_source.rs Added CachedSourceChunks wrapper, commented out test, removed rope() usage

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@SyMind SyMind force-pushed the perf-remove-rope branch 3 times, most recently from a1bad22 to 10c1172 Compare November 6, 2025 07:58
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 6, 2025

CodSpeed Performance Report

Merging #200 will improve performances by 49.63%

Comparing perf-remove-rope (a6acd0e) with main (7b7842a)

Summary

⚡ 1 improvement
✅ 10 untouched
⏩ 6 skipped1

Benchmarks breakdown

Benchmark BASE HEAD Change
complex_replace_source_map_cached_source_stream_chunks 54.7 ms 36.6 ms +49.63%

Footnotes

  1. 6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@SyMind SyMind force-pushed the perf-remove-rope branch 2 times, most recently from 4f17f19 to c5719e7 Compare November 6, 2025 09:01
@SyMind SyMind merged commit 446ceaf into main Nov 6, 2025
10 checks passed
@SyMind SyMind deleted the perf-remove-rope branch November 6, 2025 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants