Skip to content

PagePool::{default -> new_for_test} + temporary hack for IN_MEMORY_CONFIG / test_index_scans #2707

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Centril
Copy link
Contributor

@Centril Centril commented May 6, 2025

Description of Changes

First, PagePool::default is renamed to PagePool::new_for_test to make sure ::default() isn't used anywhere.
This was useful when debugging the issue.

Second, IN_MEMORY_CONFIG in crates/testing/src/modules.rs is changed so that page_pool_max_size: Some(size_of::<Page>()), which is the minimum possible. This makes the failures in test_index_scans go away. On my i7-7700K/64GB the threshold for the test failing seems to be at 1 << 26 but it passes on 1 << 25. As to why a page pool >= 64 MiB would cause such a slow down is unclear to me as of yet.

Third, the default page pool size is now Some(size_of::<Page>()) which effectively disables page pooling.

API and ABI breaking changes

None

Expected complexity level and risk

1

Testing

This fixes a test.

@Centril Centril force-pushed the centril/hack-test_index_scans branch from 20d2cc3 to 464bbe4 Compare May 6, 2025 11:56
@Centril Centril requested review from jsdt and gefjon May 6, 2025 11:56
@Centril Centril force-pushed the centril/hack-test_index_scans branch from 464bbe4 to cab6b01 Compare May 6, 2025 12:06
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

  • Needs companion PR to fix internal tests.
  • Should we be using a smaller page pool everywhere, not just in tests? Do the page pool metrics have anything useful to tell us here?

The code changes look fine and I'm willing to approve this, but I'm not super excited to merge it without knowing whether using the same smaller pool everywhere would win us two orders of magnitude of performance somehow.

@Centril Centril force-pushed the centril/hack-test_index_scans branch from cab6b01 to ee91dbf Compare May 7, 2025 14:13
@Centril
Copy link
Contributor Author

Centril commented May 7, 2025

Needs companion PR to fix internal tests.

https://github.com/clockworklabs/SpacetimeDBPrivate/pull/1691

Should we be using a smaller page pool everywhere, not just in tests? Do the page pool metrics have anything useful to tell us here?

The metrics, if I'm reading them correctly, suggests that we were storing about 40k pages in the pool, whereas the limit is about 131k pages. Might be good to shrink to 4 GiB instead of 8 GiB as the default? But I would suggest that we do such changes in follow ups to unblock CI/other PRs.

@Centril Centril requested a review from gefjon May 7, 2025 14:25
@Centril
Copy link
Contributor Author

Centril commented May 8, 2025

We decided to disable the page pool in the host for now.

@Centril Centril force-pushed the centril/hack-test_index_scans branch from d3aa7b6 to c160fe6 Compare May 8, 2025 09:10
@Centril
Copy link
Contributor Author

Centril commented May 8, 2025

@coolreader18 the smoketests failing don't seem to have anything to do with this PR but look related to #2644?

@gefjon
Copy link
Contributor

gefjon commented May 8, 2025

the smoketests failing don't seem to have anything to do with this PR but look related to #2644?

I think these are related to #2714 , actually.

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.

2 participants