Stream: git-wasmtime

Topic: wasmtime / PR #2064 Reuse ELF image from code_memory

Wasmtime GitHub notifications bot (Jul 22 2020 at 18:49):

yurydelendik opened PR #2064 from code-memory-reuse to main:

Optimization for #2020

Changes:

• Places entire ELF image into code_memory
• Use code_memory data as a source for wasm_module_serialize instead keeping pristine copy in memory
• Use code_memory data as GDB JIT image
• Sanitizes serialized image from current address information
• Moves ELF patching logic into the wasmtime-obj crate, also refactored for adding other platforms.

Wasmtime GitHub notifications bot (Jul 23 2020 at 17:43):

yurydelendik updated PR #2064 from code-memory-reuse to main:

Optimization for #2020

Changes:

• Places entire ELF image into code_memory
• Use code_memory data as a source for wasm_module_serialize instead keeping pristine copy in memory
• Use code_memory data as GDB JIT image
• Sanitizes serialized image from current address information
• Moves ELF patching logic into the wasmtime-obj crate, also refactored for adding other platforms.

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton created PR Review Comment:

While you're here, mind going ahead and changing this to an anyhow::Result to avoid using String as an error?

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton created PR Review 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.

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton created PR Review Comment:

I think we'll probably want to resolve this before landing

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton created PR Review 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)

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton created PR Review Comment:

Could the unsafe be avoided here by reading the second data directly from the section from the iterator?

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton created PR Review 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?

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton created PR Review Comment:

Would it be possible to not add unsafe here? Could the code bytes be read from a safe method?

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton created PR Review Comment:

Could the unsafety here be encapsulated in a safe method?

Wasmtime GitHub notifications bot (Aug 03 2020 at 21:35):

alexcrichton created PR Review Comment:

How come these errors are ignored?

Wasmtime GitHub notifications bot (Aug 07 2020 at 19:30):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Aug 07 2020 at 19:30):

yurydelendik created PR Review Comment:

That is the main reason for code duplication -- I can add a comment about setters.

Wasmtime GitHub notifications bot (Mar 01 2024 at 19:07):

alexcrichton closed without merge PR #2064.

Wasmtime GitHub notifications bot (Mar 01 2024 at 19:07):

alexcrichton commented on PR #2064:

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 main anyway over time.

Last updated: Nov 22 2024 at 16:03 UTC