Skip to content

feat: add new_with_capacity to Assembler#115

Merged
CensoredUsername merged 2 commits into
CensoredUsername:devfrom
nhtyy:n/assembler-with-capacity
Jul 7, 2025
Merged

feat: add new_with_capacity to Assembler#115
CensoredUsername merged 2 commits into
CensoredUsername:devfrom
nhtyy:n/assembler-with-capacity

Conversation

@nhtyy

@nhtyy nhtyy commented Jun 30, 2025

Copy link
Copy Markdown
Contributor

I think most use cases do have a lower bound on the number of emitted instructions, and using VecAssembler requires extra steps to make the code executable.

@CensoredUsername

Copy link
Copy Markdown
Owner

Hi! Thank you for the contribution. I understand the need for such an API. However, I have some comments.

First, this API would now take a certain amount of instructions as an argument. I would recommend just taking the size as bytes. You've now assumed an instruction size of 7 bytes, which could be correct for x64/x86, but would be wildly incorrect for the riscv/aarch64 backends. This is a detail is probably best exposed to the programmer to begin with, rather than assuming a certain average instruction size.

Second, I would strongly recommend however that MemoryManager is always initialized with a multiple of the memory page size. Memory permissions can only be changed on a page size basis for most platforms, so reserving anything less than a full page doesn't actually save anything. R::page_size() contains the size of a memory page in bytes that you need.

@nhtyy

nhtyy commented Jun 30, 2025

Copy link
Copy Markdown
Contributor Author

Pushed a commit that should fix your concerns :)

@CensoredUsername CensoredUsername changed the base branch from master to dev July 7, 2025 22:20
@CensoredUsername CensoredUsername merged commit b816ac2 into CensoredUsername:dev Jul 7, 2025
1 check passed
@CensoredUsername

Copy link
Copy Markdown
Owner

Thanks! I'll cut a release with this in a week or so.

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