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

Eliminated shared runtime library #169

Closed
wants to merge 8 commits into from

Conversation

jjthomas
Copy link
Collaborator

Compile times may have gone up ... tests seem to take a bit longer to run. @mateiz maybe you can measure this?

weld_module_free and weld_module_mem_free are now used to free memory (they free all memory allocated by the module, which is what weld_value_free did before), and weld_module_memory_usage is used to determine a module's total memory usage. weld_value_free and weld_value_memory_usage now do nothing. I think we should implement these later once we figure out how to correctly free and determine the memory usage of a single value.

Copy link
Collaborator

@sppalkia sppalkia left a comment

Choose a reason for hiding this comment

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

This looks good - can you also update the Python bindings, and also add a description of what the few functions do?

What's the difference between the module_mem_free and the old value free? I think the old value free basically just freed all memory that couldn't be freed during runtime right? I guess now, it frees memory for all runs if you run a module more than once?

@jjthomas
Copy link
Collaborator Author

Yep, that's right. I added some comments to the new functions clarifying this. The Python bindings have been updated.

Copy link
Collaborator

@mateiz mateiz left a comment

Choose a reason for hiding this comment

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

Hey James, this is nice but the API doesn't 100% make sense to me now. Can each module still have multiple runs? If so, how come you can't free or request memory usage for one run at a time?

@@ -41,6 +41,8 @@ $ ln -s /usr/local/bin/llvm-config-3.8 /usr/local/bin/llvm-config

To make sure this worked correctly, run `llvm-config --version`. You should see `3.8.x` or newer.

Enter the `weld_rt/cpp` directory and try running `make`. If the command fails with errors related to missing header files, you may need to install XCode and/or XCode Command Line Tools. Run `xcode-select --install` to do this.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a new step? I thought we had cargo run this and fail if it didn't work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not a new step, it's a one-time thing the user would do to check if they need to get XCode/XCode Command Line Tools.

@@ -66,14 +67,16 @@ fn many_threads_conf() -> *mut WeldConf {
unsafe fn _compile_and_run<T>(code: &str,
conf: *mut WeldConf,
ptr: &T)
-> Result<*mut WeldValue, *mut WeldError> {
-> Result<(*mut WeldValue, *mut WeldModule), *mut WeldError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this return? Update the comment above

weld/lib.rs Outdated
pub unsafe extern "C" fn weld_module_mem_free(module: *mut WeldModule) {
let module = &mut *module;
module.run_named("rt_run_free", 0).unwrap();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So is the idea that you call weld_module_mem_free to free stuff it allocated but leave the module around, and weld_module_free to delete the whole module? It seems a bit weird to tie memory management to a module. For example, can you call the same module from multiple threads now?

@mateiz
Copy link
Collaborator

mateiz commented Jul 19, 2017

Basically people will want to compile a module once and then call it many times, possibly in different threads. Having only a module-level free function won't make sense for that.

@jjthomas
Copy link
Collaborator Author

@mateiz @sppalkia fixed up the organization and naming of the runtime library, support for run_id is there as well

@sppalkia
Copy link
Collaborator

@jjthomas for some reason on Travis, the include_bytes! call can't find libweldrt - is it being compiled before the rust stuff?

@mateiz
Copy link
Collaborator

mateiz commented Jul 21, 2017

@sppalkia I'm in the process of debugging this here: #178. I think libweldrt did not build correctly on Linux with this PR, but even when it does, there are problems. The new runtime code depends on the C++ standard library, but we don't have that library loaded in Python on Linux for whatever reason. I think it's a dynamically linked library.

@mateiz
Copy link
Collaborator

mateiz commented Jul 21, 2017

BTW the reason it did not build is because errno is a macro in C++ (!) so you can't have variables named errno: c262c9e

@jjthomas jjthomas closed this Jul 21, 2017
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.

None yet

3 participants