Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't copy compressed slot to compressed batch struct #6806

Merged
merged 11 commits into from Apr 11, 2024

Conversation

akuzm
Copy link
Member

@akuzm akuzm commented Apr 4, 2024

There is overhead associated with copying the heap tuple and (un)pinning the respective heap buffers, which becomes apparent in vectorized aggregation.

Instead of this, it is enough to copy the by-reference segmentby values to the per-batch context.

Also we have to copy in the rare case where the compressed data is inlined into the compressed row and not toasted.

No changes in performance, as expected: https://grafana.ops.savannah-dev.timescale.com/d/fasYic_4z/compare-akuzm?orgId=1&var-branch=All&var-run1=3407&var-run2=3411&var-threshold=0&var-use_historical_thresholds=true&var-threshold_expression=2.5%20%2A%20percentile_cont%280.90%29&var-exact_suite_version=false

Disable-check: force-changelog-file

There is overhead associated with copying the heap tuple and (un)pinning
the respective heap buffers, which becomes apparent in vectorized
aggregation.
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 84.78261% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 80.92%. Comparing base (59f50f2) to head (ab82a22).
Report is 97 commits behind head on main.

Files Patch % Lines
tsl/src/nodes/decompress_chunk/compressed_batch.c 81.08% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6806      +/-   ##
==========================================
+ Coverage   80.06%   80.92%   +0.85%     
==========================================
  Files         190      194       +4     
  Lines       37181    36854     -327     
  Branches     9450     9645     +195     
==========================================
+ Hits        29770    29823      +53     
- Misses       2997     3182     +185     
+ Partials     4414     3849     -565     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@akuzm akuzm marked this pull request as ready for review April 5, 2024 11:45
Comment on lines 839 to 841
// fprintf(stderr, "segmentby column [%d]: value %p, null %d\n",
// attr, (void*) decompressed_tuple->tts_values[attr],
// decompressed_tuple->tts_isnull[attr]);
Copy link
Member

Choose a reason for hiding this comment

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

probably wanna remove that

Copy link
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

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

Should remove the commented out debug statements before merging

@akuzm akuzm enabled auto-merge (squash) April 11, 2024 11:13
@akuzm akuzm merged commit 610db31 into timescale:main Apr 11, 2024
41 of 44 checks passed
@akuzm akuzm deleted the no-compressed-copy branch April 11, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants