-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
WIP: libuv rewrite and Windows port #521
Conversation
Wow, looks like a lot of work. Is this just the libuv patch, or the full Windows patch? Are all the |
It's the full windows patch. The cmake files are necessary if the normal build system doesn't work (which can happen quite easily on windows and (even more importantly) if we want to move move to MSVC (which has better code generation/optimization on windows). Furthermore cmake makes it quite easy to do out-of-source builds which can be useful at times (e.g. to have a separate set of debug/release libraries - I used this during testing because with certain llvm flags, you can actually debug the JIT [That'l be possible in release mode with the next release of LLVM, too]). So no, not stricly necessary to build, but they don't hurt either and can be very useful if make doesn't work out of the box. |
@loladiro what's the simplest way I can try this out on OS X on my setup? Should I fork your repository? |
should work just fine |
Also, |
Fixed (I used root2 to switch between debug/release builds of llvm). Thanks |
A couple more problems with the Makefile:
After cd'ing into
Here's the full dump:
|
Ah yes, I saw that upstream removed that feature temporarily a few hours ago, because it wasn't working yet (we're not using it anyway, but of course we have to tell it not to.) |
Ok, after deleting the offending line,
so i added
|
Wow this is amazing. Haven't looked through all of it yet, but I noticed some I/O-related stuff. Less important, I'm worried that |
I'll have a look at it tonight |
@nolta That assertion failure is part of a problem that is caused by the differences of 32 vs. 64 bit (I was using ming32 on windows). I should have a fix soon.
Any other ideas? |
The Yes, the GC could free the array. We will need a way to protect specific objects from GC until they are released. There is a function |
The problem here is doing the type inference and multiple dispatch. I wanted you to have a look at that anyway (my solution was just temporary). I rewrote it a little bit, but do you have any ideas how to pass arbitrary Julia functions to C and getting the types right? I'm sure you're way more qualified than I to write that code. Let me know what you think.
|
Any julia function (full generic function, method, whatever) is callable with the exact same Code like this:
is dangerous since it assumes the type is a pointer-sized bits type, but the actual type was chosen elsewhere. Instead it should do something like Also |
@JeffBezanson JL_CALLABLE(jl_f_make_callback)
{
JL_TYPECHK("make_callback",function,args[0]);
JL_TYPECHK("make_callback",tuple,args[1]);
jl_callback_t *cb = malloc(sizeof(jl_callback_t));
cb->function=(jl_function_t*)args[0];
cb->types=(jl_tuple_t*)args[1];
cb->state=nargs>2?args[2]:0;
#ifdef ENVIRONMENT64
jl_value_t *t = jl_box_int64((void *)cb);
#else
jl_value_t *t = jl_box_int32((void *)cb);
#endif
JL_GC_POP();
return t;
}
void jl_callback_call(jl_callback_t *cb,...)
{
int l = cb->types->length+((cb->state!=0)?1:0);
jl_value_t **argv = calloc(l,sizeof(jl_value_t*));
JL_GC_PUSHARGS(argv,l);
va_list argp;
va_start(argp,cb);
jl_value_t *v;
int i=0;
if(cb->state!=0) argv[i++]=cb->state;
for(i; i<l; ++i) {
jl_type_t *t = (jl_type_t*)jl_tupleref(cb->types,i-(cb->state>=0?1:0));
if(t==
#ifdef ENVIRONMENT64
jl_pointer_type||t==
#endif
jl_int64_type) {
v = jl_box_int64(va_arg(argp,int64_t));
} else if(t==
#ifdef ENVIRONMENT32
jl_pointer_type||t==
#endif
jl_int32_type) {
v = jl_box_int32(va_arg(argp,int32_t));
} else {
jl_error("callback: only Ints and Pointers are supported at this time");
}
v->type=t;
argv[i]=v;
}
jl_apply(cb->function,(jl_value_t**)argv,l);
JL_GC_POP();
} |
Much better. But next, observe that making the callback struct is essentially the same as allocating a closure --- a function plus arguments to pass. For example, The other problem with the jl_callback_t is our GC can't find it and doesn't know its layout. If you use a normal julia object, all you have to do is make sure it's rooted while it might be needed by libuv, and things will be fine. |
Yes, I learned about closures after I had already written that originally. I'll rework it to use closures. |
Probably not; if necessary you can define the callback type in julia and access it from C using |
I cloned the repo, and just tried building on OS X. This seems like a simple build issue. Will try fixing it later in the day.
|
Yes, I already fixed it in my local repo. I'll push shortly: I'm just sorting out some callback stuff. |
Ok, I'm figuring something out using closures, but the syntax is a mess (I guess I'll write some kind of macro to make it better). I'll probably have a rough prototype of that tomorrow. |
@loladiro I tried pushing to your fork, but I didn't have permission. I expected it to generate a pull request. Anyways, here is what I did to get uv to build and install. You can replace what you have with:
|
Done. Also, I gave you, Jeff and Stefan commit access to my fork |
@JeffBezanson I've been trying to figure the closure thing out. It works fine if I don't have to pass any parameters back such as
But then there are also situations in which I do have to pass parameters back. I tried something like:
However, I can't seem to figure out how to access the types of the parameters. I tried f->linfo->sparams, but that does not seem to be set. What would be the best way to get the type information? |
@JeffBezanson I'm still trying to figure out how to get the type information (I haven't had much time to work on it due to midterms), would
be the way to do that? And if so, how do I get the type information from the returned array of variables? |
No, I don't think you should do anything like that. If they're pointers, just package them using |
What if I want to have ints on a 64 bit platform? I need to get at least some sort of type information to know what kind of C type is expected. Originally I had an additional type field in jl_callback_t, so that I would know what kind of c type to get: I guess I could pass pointers to the c values to julia, but then there still remains the problem of how many arguments the Julia function expects. Now, I could make that an additional argument to jl_callback_call, but that's unnecessary duplication of information, given that we already of to add another layer of indirection since we can't directly pass ints. |
At the point where
you know the types of x and y. There's your type information. I understand |
Any changes made to our copy of libuv should definitely be considered for merging back upstream though. If we need it, someone else may also, and, of course, it would be better if we didn't have to maintain a separate patched version :-) |
Right, once we figure out how do to it properly I'll shoot a quick email to the maintainers of libuv, to see if they can use any of our additions (I doubt it though as it's not used within node, but we'll see). |
I think the libuv maintainers are well aware that Node is not their only customer, and are all for more users of their library. Just justify it well and pay attention to the review feedback, same as any other upstream. Worst case, we keep the fork. |
My understanding is that the spawn/fork situation isn't the most polished part of libuv. They probably need a fix and don't know it yet. |
It doesn't seem too too bad, it just wasn't made to do anything complex. So it'll take a bit of work to get it ready for our needs. I'm tracking the primary issues here Keno#5, if you care to help comment. Is the following behavior intentional (i.e. desired) or accidental? I ask since in the current implementation of the libuv port, this is currently broken (in short, it fails trying to make an infinite pipeline of dup processes). The question is, should it be fixed to duplicate the behavior described in the manual or does it make more sense if this acts like the following (which is more closely what it is currently trying to do): -Jameson On Apr 21, 2012, at 4:43 AM, Joshua Holbrook wrote:
|
Just want to jot down a few notes from the meeting yesterday:
|
Another one:
|
Yeah, definitely. We use it for process management at work so one of my coworkers had to make it able to detach processes. From what I remember they were definitely open to improvements but most of the core efforts were going towards tcp (natch) so it was/is an area that can use some love. |
Also: I heard that the libuv peeps are also talking about fixing the stream issue right now so almost definitely talk to them. :) |
See: Keno#5 (comment) |
I just updated this pull request against the current master. The diff now contains almost exclusively changes to the Julia codebase itself (The diff dropped from 11,000 lines to ~5,700 lines). Also note that although I will keep this pull request up, it will not be merged but rather I will continue in the current form of slowly adding parts of it to master on a feature-by-feature basis. |
How can I build ff574dc on MinGW? It seems that the variable WGET_DASH_O isn't set on MinGW and the following error occurs.
Setting the variable WGET_DASH_O manually, the following error occurs.
|
sry about that. We recently changed the build system and I did not yet adjust the build scripts for windows. I'll do that tomorrow. If you want to try it today, you can use commit 3aa49c5 , but you'd have to recompile once I fix it tomorrow. Again, I apologize for the inconvenience. |
Thanks. I tried 3aa49c5 and it has been built successfully. But there are some problems remaining.
julia> min(1.0, 2.0)
Error: Symbol Could not be found fmin (-1:127)
julia> f = open("README.md")
IOStream(<file README.md>)
julia> readline(f)
"�h\U-ffdbc0\0\0\x01\0\x01\0\0\0\0\0\x13\0\0\0P\0\0\0\0\0\0\0<a name=\"banner\"
/>\n\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0
(snip)
0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\
0\0\0\0\x01\0\0Ћ\x02\x1f\0\0invalid UTF-8 character index
diff --git a/ui/repl-readline.c b/ui/repl-readline.c
index adfcdf3..071c3ff 100644
--- a/ui/repl-readline.c
+++ b/ui/repl-readline.c
@@ -522,7 +522,9 @@ static void init_rl(void)
rl_bind_key_in_map('\005', line_end_callback, keymaps[i]);
rl_bind_key_in_map('\002', left_callback, keymaps[i]);
rl_bind_key_in_map('\006', right_callback, keymaps[i]);
- rl_bind_keyseq_in_map("\033[3~",delete_callback, keymaps[i]);
+ rl_bind_keyseq_in_map("\e[1~", line_start_callback, keymaps[i]);
+ rl_bind_keyseq_in_map("\e[4~", line_end_callback, keymaps[i]);
+ rl_bind_keyseq_in_map("\e[3~", delete_callback, keymaps[i]);
rl_bind_keyseq_in_map("\e[A", up_callback, keymaps[i]);
rl_bind_keyseq_in_map("\e[B", down_callback, keymaps[i]);
rl_bind_keyseq_in_map("\e[D", left_callback, keymaps[i]); |
The |
Given the huge demand for the windows port, is it possible for us to start merging bits and pieces that will let people try out the windows port, even though not all the featuers may be ready? |
I've already been merging bits and pieces all along. There's one bug that's currently blocking a binary release, but as soon as I get that figured out, I'll upload it. |
Ignore this. It wasn't actually merged. I accidentally force pushed JuliaLang/master onto my own repository (thus there were no changes anymore). I'll see if I can reopen somehow. |
Ok, it seems that I can't reopen. I'll just leave it as is until we're ready to merge and the figure things out from there. |
It appears that's the case for pull requests. You could put up a new one and crossreference to this issue if you want to keep this visible. |
Can you try |
Stdlib: SparseArrays URL: https://github.com/JuliaSparse/SparseArrays.jl.git Stdlib branch: main Julia branch: master Old commit: cb602d7 New commit: a09f90b Julia version: 1.12.0-DEV SparseArrays version: 1.12.0 Bump invoked by: @dkarrasch Powered by: [BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl) Diff: JuliaSparse/SparseArrays.jl@cb602d7...a09f90b ``` $ git log --oneline cb602d7..a09f90b a09f90b Adjust matvec and matmatmul! to new internal LinAlg interface (#519) 3b30333 ci: run aqua test as a standalone ci job (#537) df0a154 Add versioned Manifest files to .gitignore (#534) 4606755 Extend `copytrito!` for a sparse source (#533) 33fbc75 SparseMatrixCSC constructor with a Tuple of Integers (#523) 08d6ae1 CI: don't run `threads` tests in Windows GHA CI (attempt 2) (#530) 7408e4b Revert "Don't fail CI if codecov upload fails." (#527) 287e406 Bump julia-actions/setup-julia from 1 to 2 (#524) b5de0da Don't fail CI if codecov upload fails. (#525) 78dde4c cast to Float64 directly instead of using float (#521) a5e95ec CI: Add Apple Silicon (macOS aarch64) to the CI matrix (#505) ``` Co-authored-by: Dilum Aluthge <[email protected]>
…aLang#54406) Stdlib: SparseArrays URL: https://github.com/JuliaSparse/SparseArrays.jl.git Stdlib branch: main Julia branch: master Old commit: cb602d7 New commit: a09f90b Julia version: 1.12.0-DEV SparseArrays version: 1.12.0 Bump invoked by: @dkarrasch Powered by: [BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl) Diff: JuliaSparse/SparseArrays.jl@cb602d7...a09f90b ``` $ git log --oneline cb602d7..a09f90b a09f90b Adjust matvec and matmatmul! to new internal LinAlg interface (JuliaLang#519) 3b30333 ci: run aqua test as a standalone ci job (JuliaLang#537) df0a154 Add versioned Manifest files to .gitignore (JuliaLang#534) 4606755 Extend `copytrito!` for a sparse source (JuliaLang#533) 33fbc75 SparseMatrixCSC constructor with a Tuple of Integers (JuliaLang#523) 08d6ae1 CI: don't run `threads` tests in Windows GHA CI (attempt 2) (JuliaLang#530) 7408e4b Revert "Don't fail CI if codecov upload fails." (JuliaLang#527) 287e406 Bump julia-actions/setup-julia from 1 to 2 (JuliaLang#524) b5de0da Don't fail CI if codecov upload fails. (JuliaLang#525) 78dde4c cast to Float64 directly instead of using float (JuliaLang#521) a5e95ec CI: Add Apple Silicon (macOS aarch64) to the CI matrix (JuliaLang#505) ``` Co-authored-by: Dilum Aluthge <[email protected]>
…aLang#54406) Stdlib: SparseArrays URL: https://github.com/JuliaSparse/SparseArrays.jl.git Stdlib branch: main Julia branch: master Old commit: cb602d7 New commit: a09f90b Julia version: 1.12.0-DEV SparseArrays version: 1.12.0 Bump invoked by: @dkarrasch Powered by: [BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl) Diff: JuliaSparse/SparseArrays.jl@cb602d7...a09f90b ``` $ git log --oneline cb602d7..a09f90b a09f90b Adjust matvec and matmatmul! to new internal LinAlg interface (JuliaLang#519) 3b30333 ci: run aqua test as a standalone ci job (JuliaLang#537) df0a154 Add versioned Manifest files to .gitignore (JuliaLang#534) 4606755 Extend `copytrito!` for a sparse source (JuliaLang#533) 33fbc75 SparseMatrixCSC constructor with a Tuple of Integers (JuliaLang#523) 08d6ae1 CI: don't run `threads` tests in Windows GHA CI (attempt 2) (JuliaLang#530) 7408e4b Revert "Don't fail CI if codecov upload fails." (JuliaLang#527) 287e406 Bump julia-actions/setup-julia from 1 to 2 (JuliaLang#524) b5de0da Don't fail CI if codecov upload fails. (JuliaLang#525) 78dde4c cast to Float64 directly instead of using float (JuliaLang#521) a5e95ec CI: Add Apple Silicon (macOS aarch64) to the CI matrix (JuliaLang#505) ``` Co-authored-by: Dilum Aluthge <[email protected]>
WARNING: Do not merge yet. I opened this pull request to have a central place for review/discussion.
The rewrite of Julia based on libuv (supporting Win,Linux and OS X) is getting more and more stable. I need to do some more testing, but now that all major features are implemented, I wanted to open up review/discussion. Fire away!