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

sstables: parse(summary): reserve positions vector #21767

Closed

Conversation

bhalevy
Copy link
Member

@bhalevy bhalevy commented Dec 3, 2024

We know the number of positions in advance
so reserve the chunked_vector capacity for that.

  • Small optimization, no backport needed

@scylladb-promoter
Copy link
Contributor

🔴 CI State: FAILURE

✅ - Build
❌ - Docker Test
✅ - dtest with topology changes
✅ - dtest with tablets
✅ - dtest
✅ - Unit Tests

Build Details:

  • Duration: 4 hr 1 min
  • Builder: spider7.cloudius-systems.com

@@ -507,7 +507,7 @@ future<> parse(const schema& schema, sstable_version_types v, random_access_read

// Positions are encoded in little-endian.
auto b = buf.get();
s.positions = utils::chunked_vector<pos_type>();
s.positions.reserve(s.header.size + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like we wanted to reset the positions to empty vector, in case it had content.
I see we don't do this consistently with all members, but maybe we should still keep the reset just in case.

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, I'll make sure it's still needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see where the summary component is reused, so I don't think this resetting is required.
We only call parse from

co_return co_await read_simple<component_type::Summary>(_components->summary);

After checking

scylladb/sstables/sstables.cc

Lines 1308 to 1310 in cd2a2bd

if (_components->summary) {
co_return;
}

So we don't reuse the object.

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to add an explanation in the commit why it can go away. I'd expect a separate commit for removing the reset, but maybe that's bikeshedding since it's a simple change.

Copy link
Contributor

@nyh nyh Dec 26, 2024

Choose a reason for hiding this comment

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

I would add a comment in front of this function saying it can only be called once.
But, wouldn't it be easier just to allow it to be called twice? I.e., leave the reset as in the old code - and then add the reserve() you wanted to add?

Copy link
Member Author

Choose a reason for hiding this comment

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

But we didn't reset s.entries before so it was never supposed to work this way.
We don't need to invent new use cases to excuse some old artifact.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point.

We know the number of positions in advance
so reserve the chunked_vector capacity for that.

Note: reservation replaces the existing reset of the
positions member.  This is safe since we parse the summary
only once as sstable::read_summary() returns early
if the summary component is already populated.

Signed-off-by: Benny Halevy <[email protected]>
@bhalevy bhalevy force-pushed the sstables-summary-reserve-positions branch from 7ee6c18 to f86d498 Compare December 24, 2024 12:51
@bhalevy
Copy link
Member Author

bhalevy commented Dec 24, 2024

  • rebased
  • added note to git commit message about why it is safe to drop thew existing reset of positions.

@scylladb-promoter
Copy link
Contributor

🟢 CI State: SUCCESS

✅ - Build
✅ - dtest with topology changes
✅ - dtest with tablets
✅ - dtest
✅ - Docker Test
✅ - Offline-installer Artifact Tests
✅ - Unit Tests

Build Details:

  • Duration: 3 hr 45 min
  • Builder: spider5.cloudius-systems.com

@bhalevy bhalevy added the status/merge candidate Item needs maintainer attention label Dec 26, 2024
@bhalevy
Copy link
Member Author

bhalevy commented Dec 26, 2024

@scylladb/scylla-maint please consider merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/none Backport is not required enhancement promoted-to-master status/merge candidate Item needs maintainer attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants