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

Fix issue with memory coalescing #221

Merged
merged 5 commits into from
Jul 15, 2024
Merged

Fix issue with memory coalescing #221

merged 5 commits into from
Jul 15, 2024

Conversation

l-kent
Copy link
Contributor

@l-kent l-kent commented Jun 13, 2024

Fixes #201

I have an idea for a more sophisticated fix too but it will take more effort so this will work for the time being.

@l-kent l-kent requested a review from ailrst June 13, 2024 23:19
Copy link
Contributor

@ailrst ailrst left a comment

Choose a reason for hiding this comment

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

Might be a problem with the aligned section loop if I'm reading it right, it probably deserves a comment on the whole function as well. It probably wouldn't be too hard to add a unit test for this function but I'm not requiring it.

// Combine the byte constants into a 64-bit value
val sum: BigInt =
(0 until 8).foldLeft(BigInt(0))((x, y) => x + (s.bytes(b + y).value * (BigInt(2).pow(y * 8))))
// section is a multiple of 64-bits and appropriately aligned
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem accurate? It will coalesce any internal 64-bit bit multiple aligned subsection and concatenate the unaligned prefix and suffix.

// Section that is safe to convert to 64-bit values
val section = for (b <- aligned until alignedEnd by 8) yield {
// Combine the byte constants into a 64-bit value
val sum: BigInt =
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a concat not really a sum although it uses sums

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This name was just taken from the original implementation

val alignedEnd = s.bytes.size - alignedSizeMultiple

// Section that is safe to convert to 64-bit values
val section = for (b <- aligned until alignedEnd by 8) yield {
Copy link
Contributor

@ailrst ailrst Jun 24, 2024

Choose a reason for hiding this comment

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

Should this be alignedStart until alignedEnd by 8?

Would be good to distinguish alignedStart = address, and alignedEnd = region index

@l-kent l-kent merged commit 8135d0c into main Jul 15, 2024
0 of 2 checks passed
@l-kent l-kent deleted the memory-consolidation branch July 16, 2024 00:54
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.

Memory consolidation doesn't happen
2 participants