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

SimpleJIT hot code swapping #2403

Merged
merged 12 commits into from
Dec 3, 2020

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Nov 12, 2020

This makes it possible to change functions when using SimpleJIT without having to recompile everything or even restart the jitted code. When a changed function is currently on the stack, the old version will be returned to, but all new calls will be redirected to the new version. It is not yet possible to swap data objects.

This is a prerequisite of https://github.com/bjorn3/rustc_codegen_cranelift/issues/1087.

I have tested this PR by implementing lazy compilation in cg_clif as suggested by @flodiebold in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/cranelift.20backend.20work/near/187645798 in February.

r? @pchickey (By the way, is it deliberate that you are marked as unavailable on zulip? I sent you a PM)
cc https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift/topic/hot.20code.20swapping.20for.20simplejit/near/216403611

@bjorn3 bjorn3 force-pushed the simplejit_hot_swapping branch from bbc2afb to 86d3dc9 Compare November 12, 2020 18:39
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:module labels Nov 12, 2020
@pchickey pchickey self-requested a review November 12, 2020 19:02
Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

Thank you for this work - it may be time to drop the Simple from the name of this JIT ;) ! This is a bit over my head right now, I need to spend more time reading and understanding this and may need to pull in another reviewer who is more familiar.

@@ -129,9 +131,13 @@ pub struct SimpleJITModule {
libcall_names: Box<dyn Fn(ir::LibCall) -> String>,
memory: MemoryHandle,
declarations: ModuleDeclarations,
function_got_entries: SecondaryMap<FuncId, Option<NonNull<*const u8>>>,
function_plt_entries: SecondaryMap<FuncId, Option<NonNull<[u8; 16]>>>,
Copy link
Contributor

@pchickey pchickey Nov 12, 2020

Choose a reason for hiding this comment

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

I'm not clear on how 16 bytes was arrived at, and how platform specific this is - is it due to 16-byte function call alignment on x86_64? does some other sequence of instructions have to fit in here besides the plt_val at line 179?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future I may extend it to be more like the lazy symbol binding used by ELF. In that case the GOT entry will initially point just after the jump instruction in the PLT. After this jump instruction it pushes a specific value to the stack depending on which PLT entry it is and then it jumps to a function common between all entries in a module. This function then resolves the symbol. This avoids having to create a new wrapper function for each lazily compiled function.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. a comment to this effect would be appreciated :)

@bjorn3
Copy link
Contributor Author

bjorn3 commented Nov 13, 2020

It seems that the new x64 backend doesn't have support for position independent code.

@cfallin
Copy link
Member

cfallin commented Nov 13, 2020

It seems that the new x64 backend doesn't have support for position independent code

Please do create an issue! I will look into this at some point as part of the transition-to-new-backend effort.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Nov 13, 2020

Done: #2417

Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

Dan and I walked through this today and I'm now more confident I understand this design space. I'll merge this as soon as CI gets fixed up

@bjorn3 bjorn3 force-pushed the simplejit_hot_swapping branch from 2fea300 to 937a3fd Compare December 3, 2020 18:25
@bjorn3
Copy link
Contributor Author

bjorn3 commented Dec 3, 2020

@pchickey I have restored support for non-pic code in SimpleJIT to fix x64. Hot code swapping needs to be disabled in that case though. I have tested it with cg_clif in the non-pic, pic and hotswapping configuration.

@pchickey
Copy link
Contributor

pchickey commented Dec 3, 2020

Excellent, that is even better. Want to make a follow-up PR dropping the Simple from SimpleJIT?

@pchickey pchickey merged commit 0f1dc9a into bytecodealliance:main Dec 3, 2020
@bjorn3 bjorn3 deleted the simplejit_hot_swapping branch December 3, 2020 23:01
@bjorn3
Copy link
Contributor Author

bjorn3 commented Dec 3, 2020

Sure

bjorn3 added a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Dec 4, 2020
This includes bytecodealliance/wasmtime#2403 which enables hotswapping with SimpleJIT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:module cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants