Reuse ELF image from code_memory#2064
Conversation
yurydelendik
force-pushed
the
code-memory-reuse
branch
from
ba86bc4 to
e218eab
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Ok I've left a few comments here and there, but I think I may be missing something as well? I can see the entire ELF image being copied into the code memory, but I'm having trouble figuring out the end goal ofthis PR. Is the intention that the Object isn't kept around which duplicates resident memory by having code twice? This isn't related to minimizing the number of copies, right?
(sorry I'm somewhat familiar with all these paths but not intimately familiar)
| } | ||
|
|
||
| trait ElfObject { | ||
| fn e_ident(&self) -> &Ident; |
There was a problem hiding this comment.
It looks like some of these reader methods are already on a predefined trait, could that be used instead?
(it looks like setters aren't found there though)
There was a problem hiding this comment.
That is the main reason for code duplication -- I can add a comment about setters.
| @@ -400,11 +402,9 @@ fn build_code_memory( | |||
| ), | |||
| String, | |||
There was a problem hiding this comment.
While you're here, mind going ahead and changing this to an anyhow::Result to avoid using String as an error?
| let obj = (obj.as_ptr(), obj.len()); | ||
|
|
||
| // Make all code compiled thus far executable. | ||
| // TODO publish only .text section of ELF object. |
There was a problem hiding this comment.
I think we'll probably want to resolve this before landing
| continue; | ||
| } | ||
| }; | ||
| let body = unsafe { obj_data.as_ptr().add(body_offset) } as *const VMFunctionBody; |
There was a problem hiding this comment.
Could the unsafe be avoided here by reading the second data directly from the section from the iterator?
| continue; | ||
| } | ||
| }; | ||
| let body = unsafe { obj_data.as_ptr().add(body_offset) } as *const VMFunctionBody; |
There was a problem hiding this comment.
(similar comment about unsafety as above)
Additionally this look looks basically the same as the one above, so could that be factored out? Something like a function that applies all relocations and uses a closure to calculate the reloc value.
| let mut file = match parse_elf_object_mut(bytes) { | ||
| Ok(file) => file, | ||
| Err(_) => { | ||
| return; |
There was a problem hiding this comment.
This seems like it's a bit of a scary error to omit, could we return an error here instead and use error messages to guide fixing this later?
| /// Extracts `CompilationArtifacts` from the compiled module. | ||
| pub fn to_compilation_artifacts(&self) -> CompilationArtifacts { | ||
| // Get ELF image bytes and sanitize that. | ||
| let mut obj = Vec::from(unsafe { |
There was a problem hiding this comment.
Would it be possible to not add unsafe here? Could the code bytes be read from a safe method?
| std::slice::from_raw_parts(range.start as *const u8, range.len()) | ||
| }); | ||
| drop(unlink_module(&mut obj)); | ||
| drop(sanitize_loadable_file(&mut obj)); |
There was a problem hiding this comment.
How come these errors are ignored?
| // Register GDB JIT images; initialize profiler and load the wasm module. | ||
| let dbg_jit_registration = if debug_info { | ||
| let bytes = create_dbg_image(obj.to_vec(), code_range, &module, &finished_functions)?; | ||
| let bytes = unsafe { std::slice::from_raw_parts(image_range.0, image_range.1) }; |
There was a problem hiding this comment.
Could the unsafety here be encapsulated in a safe method?
The end goal not only remove copy operations but also reduce amount of data copies as well. Currently, we are keeping pristine ELF image copy along with code_memory, which only needed for It is low priority for this optimization. Though it will probably be needed when we will emit ELF image directly into the code_memory. |
|
Ah ok I see, so it's removing some duplicates we store next to each other. Ok that makes sense to me! I'd be happy landing this since it at least improves along those lines and will be needed for future refactorings anyway. |
|
I'm going through some old PRs and this one's pretty old at this point. Enough points have changed that I don't think this is quite as applicable any more and I think much of this ended up getting landed in |
2 participants
Optimization for #2020
Changes:
wasm_module_serializeinstead keeping pristine copy in memory