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

Make heap-allocated alternatives or replacements for MemoryDump and MemoryShim #164

Open
3 tasks
gipsond opened this issue Jul 6, 2022 · 1 comment
Open
3 tasks
Assignees
Labels
➕ improvement Chores and fixes: the small things.

Comments

@gipsond
Copy link
Contributor

gipsond commented Jul 6, 2022

what

lc3_isa::util::MemoryDump and lc3_shims::memory::MemoryShim contain full LC-3 memory images (128 KiB). Currently, they allocate this large amount of memory on the stack. This has led to the assembler and TUI increasing their stack sizes. Due to the memory overhead, we already avoid using these structs on non-hosted platforms, so we could change the structs to use heap-allocated memory, or add a heap-allocated alternative. We expect this would allow the assembler and TUI to return to using default stack sizes.

steps

  • Make heap-allocated MemoryDump alternative or replacement
  • Make heap-allocated MemoryShim alternative or replacement
  • Replace usages of MemoryDump and MemoryShim (as necessary)

where

branch: imp-heap-memorydump

lc3_isa::util::MemoryDump
lc3_shims::memory::MemoryShim

relevant code:

open questions

  • Replace MemoryDump entirely? Or just provide alternative?
  • Replace MemoryShim entirely? Or just provide alternative?
@gipsond gipsond added the ➕ improvement Chores and fixes: the small things. label Jul 6, 2022
@gipsond gipsond assigned rrbutani and gipsond and unassigned rrbutani Jul 6, 2022
@rrbutani rrbutani self-assigned this Jul 7, 2022
@rrbutani
Copy link
Member

rrbutani commented Jul 9, 2022

Sorry for the delay! I'm going to go work on this now.


Just for posterity, here's some context from Slack:

an example

I think we can just fix MemoryShim and MemoryDump to do the right thing (internally house a Vec<u16> that's private and guaranteed to be the right size; implement Deref<[u16; ADDR_SPACE_SIZE]> using unwrap_unchecked, etc.)

I looked through all the usages and I think we're fine doing this
we do use MemoryDump in some APIs that are exposed in crates that need to compile with #![no_std] but we already never expected to be able to create a MemoryDump on non-hosted platforms (256K is too much to put on the stack, etc.)
I think I'll probably do:

// a separate struct so that we can make the inner field private
struct OwnedMemoryDump(Vec<Word>);

pub enum MemoryDump {
    #[cfg(feature = "std")]
    Owned(OwnedMemoryDump),
    Borrowed(&'static [u16; ADDR_SPACE_SIZE]), 
}

or something similar just so we don't have to go gate those APIs on std

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ improvement Chores and fixes: the small things.
Projects
None yet
Development

No branches or pull requests

2 participants