Skip to content

Move dirty from Context to EagerDir #886

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 2 commits into
base: main
Choose a base branch
from
Open

Move dirty from Context to EagerDir #886

wants to merge 2 commits into from

Conversation

mbouaziz
Copy link
Contributor

For typed directories, ideally we'd need to keep keys and files in directories to avoid unsafe casts. There are a few cases of keys in the Context, some are more or less problematic.

This move looks safe as dirty is only accessed in update, and only updated in writeEntry with no weird interleaving with setDir.

Ideally, I'd do the same with dirtyReaders but there are too many interleavings, so there's a risk to exist a setDir between a change and another setDir, then missing some changes.

@mbouaziz mbouaziz requested a review from skiplabsdaniel April 29, 2025 17:16
@mbouaziz mbouaziz marked this pull request as draft April 29, 2025 21:03
@mbouaziz mbouaziz marked this pull request as ready for review April 30, 2025 08:46
@mbouaziz
Copy link
Contributor Author

The failure on skdb-wasm looks non-transient, I can replicate it locally though I have no idea how this diff produces a RuntimeError: memory access out of bounds

@skiplabsdaniel
Copy link
Contributor

The failure on skdb-wasm looks non-transient, I can replicate it locally though I have no idea how this diff produces a RuntimeError: memory access out of bounds
If it's 'Write-csv memory regression test' it's probably because the diff consume more memory.

@mbouaziz
Copy link
Contributor Author

Thanks for the heads-up!
It looks like this diff has a big bad impact on memory consumption, probably because its peak temporary garbage is higher.

Base automatically changed from skstore-0428 to main May 5, 2025 07:02
mbouaziz added 2 commits May 5, 2025 09:04
This move looks safe as dirty is only accessed in update,
and only updated in writeEntry with no weird interleaving with setDir.
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