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

chore(SharedMemory): small internal refactoring #806

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

thedevbirb
Copy link
Contributor

@thedevbirb thedevbirb commented Oct 16, 2023

  • current_len is redundant -> replaced with last_checkpoint with minor performance boost
  • some renaming and refactoring done, the code is a little cleaner (imo)
  • added a couple of tests

@thedevbirb thedevbirb changed the title chore(SharedMemory): refactor and small perf improvement chore(SharedMemory): small internal refactoring Oct 16, 2023

self.current_len = new_len;
pub fn resize(&mut self, new_size: usize) {
self.buffer.reserve(usize::max(new_size, 4 * 1024));
Copy link
Collaborator

@DaniPopes DaniPopes Oct 16, 2023

Choose a reason for hiding this comment

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

resize already allocates this, actually for new_size > 4*1024 it would allocate twice.
Do you mean usize::min?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was to reserve a minimum of 4096 bytes to avoid many mini-allocations, but I agree that if new_size > 4 * 1024 then this line is useless

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, then it should be min not max

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I'm not sure: suppose new_size equals 32, if we use min then:

assert_eq!(usize::min(32, 4*1024), 32);

and we reserve 32 more bytes instead of 4096, which is not what we want.
I don't understand if I'm missing something here from what you said

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah you're right. This should be removed because Vec already does exponential allocations so this is redundant since we're starting already with 4*1024

Copy link
Member

@rakita rakita Oct 16, 2023

Choose a reason for hiding this comment

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

To be precise it should be something like .reserve(self.last_checkpoint+max(new_size,4*1024)) to allocate at least 4096 okay ignore this. As @DaniPopes said resize doubles capacity every time so it is good to rely on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! Now it should be good.

crates/interpreter/src/interpreter/shared_memory.rs Outdated Show resolved Hide resolved
Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm

@rakita rakita merged commit de3d392 into bluealloy:main Oct 18, 2023
8 checks passed
@thedevbirb thedevbirb deleted the shared_memory branch October 18, 2023 11:16
@github-actions github-actions bot mentioned this pull request Jan 12, 2024
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