softdevteam / yksom

Other
8 stars 6 forks source link

Add primitives for Krun #190

Closed jacob-hughes closed 4 years ago

jacob-hughes commented 4 years ago

This is needed in order to properly benchmark yksom with Krun as it allows us to send the necessary metrics to Krun without tearing down the VM after each in-process execution.

Right now, this will fail to build without the user passing the necessary linker flags to link libkrunruntime.so via RUSTFLAGS. At the moment this is a draft PR until we best work out how to do this nicely. I think the ideal solution is to conditionally compile krun stuff behind a benchmark-harness feature.

@ltratt do you have any other suggestions? Perhaps we can avoid this altogether if SOM has FFI support? I couldn't see this though.

ltratt commented 4 years ago

This all looks good to me! I don't have much idea about the linker flags, but https://stackoverflow.com/a/50646443 suggests a possible answer (c.f. https://doc.rust-lang.org/cargo/reference/config.html)?

jacob-hughes commented 4 years ago

I think that in general, a user would want to clone yksom into some wider Krun-like project. If we specify linker flags in the build script (or even attributes in source) then we to make assumptions about where to find libkruntime.so which I'm not sure is useful. I actually think that in this case it's acceptable to build and run yksom using RUSTFLAGS and appending to the LD_LIBRARY_PATH in the following way:

RUSTFLAGS="-l kruntime -L /path/to/libkrun/" cargo build

LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/path/to/libkrun/ ./yksom --cp SOM/Smalltalk Program.som

To make sure that we don't get linker errors when using yksom without krun, I think all this stuff should be feature gated. What do you think?

ltratt commented 4 years ago

If it can be feature-gated, excellent! If it can't... maybe the right thing to do is fork yksom into yksom_krun so that it doesn't "pollute" the main cases?

smarr commented 4 years ago

Just a note, and I have no intention to make things complicated for you.

I set up a yksom CI job for the SOM repository to see when changes to SOM would break yksom: https://github.com/SOM-st/SOM/pull/57/commits/a9e66e6ed654737b543001dbe0620df18775fe58

I'd think, the practical benefit of that is likely not out-weighting making things more complex for you guys though.

jacob-hughes commented 4 years ago

This commit (although ugly) seems to solve the problem.

Thanks @smarr, I'm pretty sure this will not break your CI job on ToT since you'll have to enable the krun_harness feature to enable the changes in this PR.

jacob-hughes commented 4 years ago

Good point. Try this: 5b546df

ltratt commented 4 years ago

Please squash.

jacob-hughes commented 4 years ago

Squashed

ltratt commented 4 years ago

bors r+

bors[bot] commented 4 years ago

Build failed:

jacob-hughes commented 4 years ago

It seems the nightly alloc API has changed again, so this should be merge-able once https://github.com/softdevteam/rboehm/pull/28 has landed.

ltratt commented 4 years ago

bors r+

bors[bot] commented 4 years ago

Build succeeded: