Skip to content

Commit

Permalink
libuv: Uses uv_async_send to dispatch callback.
Browse files Browse the repository at this point in the history 
Dispatches callback to uv_default_loop.
  • Loading branch information
@am11
am11 committed Nov 21, 2014
1 parent fa595ff commit cbcacc2
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 34 deletions.
96 changes: 63 additions & 33 deletions src/binding.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,69 +2,93 @@
#include "sass_context_wrapper.h"

char* CreateString(Local<Value> value) {
if(value->IsNull() || !value->IsString()) {
if (value->IsNull() || !value->IsString()) {
return const_cast<char*>(""); // return empty string.
}

String::Utf8Value string(value);
char *str = (char *) malloc(string.length() + 1);
char *str = (char *)malloc(string.length() + 1);
strcpy(str, *string);
return str;
}

struct Sass_Import** sass_importer(const char* file, void* cookie)
{
uv_async_t async;

void dispatched_async_uv_callback(uv_async_t *req){
NanScope();

TryCatch try_catch;
import_bag* bag = static_cast<import_bag*>(req->data);

Handle<Value> argv[] = {
NanNew<String>(file)
NanNew<String>(bag->file)
};

Local<Value> returned_value = NanNew<Value>(((NanCallback*)cookie)->Call(2, argv));
Local<Value> returned_value = NanNew<Value>(((NanCallback*)bag->cookie)->Call(1, argv));

if(returned_value->IsArray()) {
if (returned_value->IsArray()) {
Handle<Array> array = Handle<Array>::Cast(returned_value);

struct Sass_Import** incs = sass_make_import_list(array->Length());
bag->incs = sass_make_import_list(array->Length());

for(size_t i = 0; i < array->Length(); ++i) {
for (size_t i = 0; i < array->Length(); ++i) {
Local<Value> value = array->Get(i);

if(!value->IsObject())
if (!value->IsObject())
continue;

Local<Object> object = Local<Object>::Cast(value);
char* path = CreateString(object->Get(String::New("path")));
char* path = CreateString(object->Get(String::New("file")));
char* contents = CreateString(object->Get(String::New("contents")));

incs[i] = sass_make_import_entry(path, (!contents || contents[0] == '\0') ? 0 : strdup(contents), 0);
bag->incs[i] = sass_make_import_entry(path, (!contents || contents[0] == '\0') ? 0 : strdup(contents), 0);
}

return incs;
} else if(returned_value->IsObject()) {
struct Sass_Import** incs = sass_make_import_list(1);
}
else if (returned_value->IsObject()) {
bag->incs = sass_make_import_list(1);
Local<Object> object = Local<Object>::Cast(returned_value);
char* path = CreateString(object->Get(String::New("path")));
char* path = CreateString(object->Get(String::New("file")));
char* contents = CreateString(object->Get(String::New("contents")));

incs[0] = sass_make_import_entry(path, (!contents || contents[0] == '\0') ? 0 : strdup(contents), 0);
bag->incs[0] = sass_make_import_entry(path, (!contents || contents[0] == '\0') ? 0 : strdup(contents), 0);
}
else {
bag->incs = sass_make_import_list(1);
bag->incs[0] = sass_make_import_entry(bag->file, 0, 0);
}

return incs;
if (try_catch.HasCaught()) {
node::FatalException(try_catch);
}
}

struct Sass_Import** incs = sass_make_import_list(1);
struct Sass_Import** sass_importer(const char* file, void* cookie)
{
import_bag* bag = (import_bag*)calloc(1, sizeof(import_bag));

bag->cookie = cookie;
bag->file = file;

async.data = (void*)bag;
uv_async_send(&async);

// Dispatch immediately
uv_run(async.loop, UV_RUN_DEFAULT);

This comment has been minimized.

Sign in to view

Copy link
@am11

am11 Nov 21, 2014

Author Owner

@rvagg: is there a way to run the uv_async_send on default loop and mark the secondary thread as receiver, so it don't wait for main thread here?

(main discussion on this topic is at sass#530)

This comment has been minimized.

Sign in to view

Copy link
@rvagg

rvagg Nov 22, 2014

Without looking deeply into the detail here I'm pretty sure you don't want to have uv_run involved here at all. An example that may be helpful here is this pull request against NAN that introduces a "progress reporter" type API to NanAsyncWorker so that asynchronous jobs can talk to the V8 thread during their execution without waiting for completion. Note the use of uv_async_send there to simply pass off a continuation to the main thread; there's nothing more than that which needs to be done from the sender side.

This is assuming that sass_importer is not being run on the main thread, if it is then uv_async_send() is the wrong API to be using here, that's for sending in to the main thread.

This comment has been minimized.

Sign in to view

Copy link
@am11

am11 Nov 22, 2014

Author Owner

Thanks for your reply. I have made it this far:

node-sass/src/binding.cpp

Lines 70 to 84 in 670cc89

ctx_w->importer_mutex->lock();
//uv_mutex_lock(ctx_w->mutex);
// Enter critical section
ctx_w->file = strdup(file);
ctx_w->async.data = (void*)ctx_w;
uv_async_send(&ctx_w->async);
// Reassurances
//uv_mutex_lock(ctx_w->mutex);
//uv_mutex_unlock(ctx_w->mutex);
ctx_w->importer_mutex->lock();
ctx_w->importer_mutex->unlock();
return ctx_w->imports;
with @txdv's help. Now the issue is, if I use iostream's cout statement before and after the locks, it produces the desired result (the importer is working). But without those debug lines, libsass throws device or resource busy error. On debug, it suggest libsass is calling the importer twice. Apparently, this is low-level memory barrier issue now. We are using c++11, so we may take advantage of automic to streamline calls. But I am not sure if it involves making changes in libsass or not.

This comment has been minimized.

Sign in to view

Copy link
@rvagg

rvagg Nov 22, 2014

What are you locking? the only thing that really looks lockable in that block is the strdup(file) line, the rest is just shuffling bits to queue up deferred work; this looks kind of unnecessary. I suspect that your cout calls were simply introducing enough delay to prevent a timing problem -- the main thread may take its time to pick up the work you're passing it and maybe delaying the current thread until it started doing its thing was enough to make it work. I bet if you put a small sleep() in there it'd have a similar effect.

This comment has been minimized.

Sign in to view

Copy link
@am11

am11 Nov 22, 2014

Author Owner

The thing is, this function is called from libsass which was previously called by uv_queue_work. Now while uv_queue_work is expecting result from compile_it function, this function (which should pass the char* file to js-function and get the modified path back synchronously) is totally decoupled from the libuv loop.

If we don't have these look, we need uv_run after uv_async_send, which explodes with Access Violation (on returning the result from dispatcher function back to uv_run in sass_import).

Are you saying, we don't need uv_run as well as these locks and still be able to run JS function and return the result back to libsass in a synchronous manner?

This comment has been minimized.

Sign in to view

Copy link
@am11

am11 Nov 22, 2014

Author Owner

I will try with sleep now. :)

This comment has been minimized.

Sign in to view

Copy link
@am11

am11 Nov 22, 2014

Author Owner

And yes, it actually works if we have a random cout statement there. I don't know why a friction of second delay makes a difference though..

This comment has been minimized.

Sign in to view

Copy link
@am11

am11 Nov 22, 2014

Author Owner

Miraculous! With Sleep(5) it worked! With Sleep(4) and below it doesn't. Commit: b2ce0f6 😄

@mgreter, @QuLogic, is this amount of millisecond sufficient enough for cross-platform and slower CPUs?

This comment has been minimized.

Sign in to view

Copy link
@rvagg

rvagg Nov 22, 2014

@am11 the sleep trick is just to demonstrate this is a timing problem, this is not going to work in production, you have zero guarantees about how long it'll take for the main thread to pick this up and if the main thread is busy with other work then it could take a significant amount of time.

The fact is that you can't make a synchronous call from outside of the main thread into it, you can do the reverse simply by doing everything on the main thread. You're going to have to restructure the logic here to make it workable (I'm still speaking from relative ignorance about what you're trying to do).

This comment has been minimized.

Sign in to view

Copy link
@am11

am11 Nov 22, 2014

Author Owner

I just explained its working here sass#530 (comment). :D
libuv is quite beyond my area of expertise. But can't blame a guy for trying, right? ;)

This comment has been minimized.

Sign in to view

Copy link
@txdv

txdv Nov 23, 2014

Locks shouldn't have race conditions...

The Memory Barrier is literally the only thing that comes to my mind. I have to test this on my 486 just to make sure there is no reordering.

This comment has been minimized.

Sign in to view

Copy link
@am11

am11 Nov 23, 2014

Author Owner

@txdv, thanks for looking into it. 👍

Sass_Import** import = bag->incs;

incs[0] = sass_make_import_entry(file, 0, 0);
free(bag);

return incs;
return import;
}

void ExtractOptions(Local<Object> options, void* cptr, sass_context_wrapper* ctx_w, bool isFile) {
struct Sass_Context* ctx;

if (isFile) {
ctx = sass_file_context_get_context((struct Sass_File_Context*) cptr);
} else {
}
else {
ctx = sass_data_context_get_context((struct Sass_Data_Context*) cptr);
}

Expand All @@ -80,16 +104,19 @@ void ExtractOptions(Local<Object> options, void* cptr, sass_context_wrapper* ctx

if (isFile) {
ctx_w->fctx = (struct Sass_File_Context*) cptr;
} else {
}
else {
ctx_w->dctx = (struct Sass_Data_Context*) cptr;
}
ctx_w->request.data = ctx_w;
ctx_w->success_callback = new NanCallback(success_callback);
ctx_w->error_callback = new NanCallback(error_callback);
ctx_w->importer_callback = new NanCallback(importer_callback);

if(!importer_callback->IsUndefined())
if (!importer_callback->IsUndefined()){
sass_option_set_importer(sass_options, sass_make_importer(sass_importer, ctx_w->importer_callback));
uv_async_init(uv_default_loop(), &async, (uv_async_cb)dispatched_async_uv_callback);
}
}

sass_option_set_output_path(sass_options, CreateString(options->Get(NanNew("outFile"))));
Expand All @@ -109,7 +136,7 @@ void FillStatsObj(Handle<Object> stats, Sass_Context* ctx) {
char** included_files = sass_context_get_included_files(ctx);
Handle<Array> arr = NanNew<Array>();

if(included_files) {
if (included_files) {
for (int i = 0; included_files[i] != nullptr; ++i) {
arr->Set(i, NanNew<String>(included_files[i]));
}
Expand All @@ -125,14 +152,15 @@ void FillStatsObj(Handle<Object> stats, Sass_Context* ctx) {

if (sass_context_get_source_map_string(ctx)) {
source_map = NanNew<String>(sass_context_get_source_map_string(ctx));
} else {
}
else {
source_map = NanNew<String>("{}");
}

(*stats)->Set(NanNew("sourceMap"), source_map);
(*stats)->Set(NanNew("sourceMap"), source_map);
}

void MakeCallback(uv_work_t* req) {
void make_callback(uv_work_t* req) {
NanScope();

TryCatch try_catch;
Expand All @@ -144,7 +172,8 @@ void MakeCallback(uv_work_t* req) {
ctx = sass_data_context_get_context(ctx_w->dctx);
FillStatsObj(NanNew(ctx_w->stats), ctx);
error_status = sass_context_get_error_status(ctx);
} else {
}
else {
ctx = sass_file_context_get_context(ctx_w->fctx);
FillStatsObj(NanNew(ctx_w->stats), ctx);
error_status = sass_context_get_error_status(ctx);
Expand All @@ -158,7 +187,8 @@ void MakeCallback(uv_work_t* req) {
NanNew(ctx_w->stats)->Get(NanNew("sourceMap"))
};
ctx_w->success_callback->Call(2, argv);
} else {
}
else {
// if error, do callback(error)
const char* err = sass_context_get_error_json(ctx);
Local<Value> argv[] = {
Expand Down Expand Up @@ -186,7 +216,7 @@ NAN_METHOD(Render) {

ExtractOptions(options, dctx, ctx_w, false);

int status = uv_queue_work(uv_default_loop(), &ctx_w->request, compile_it, (uv_after_work_cb)MakeCallback);
int status = uv_queue_work(uv_default_loop(), &ctx_w->request, compile_it, (uv_after_work_cb)make_callback);

assert(status == 0);

Expand Down Expand Up @@ -231,7 +261,7 @@ NAN_METHOD(RenderFile) {
ctx_w->fctx = fctx;
ExtractOptions(options, fctx, ctx_w, true);

int status = uv_queue_work(uv_default_loop(), &ctx_w->request, compile_it, (uv_after_work_cb)MakeCallback);
int status = uv_queue_work(uv_default_loop(), &ctx_w->request, compile_it, (uv_after_work_cb)make_callback);

assert(status == 0);
free(input_path);
Expand Down
2 changes: 1 addition & 1 deletion src/libsass
Submodule libsass updated 42 files
+4 −1 Makefile
+6 −3 Makefile.am
+43 −31 ast.hpp
+1 −1 ast_def_macros.hpp
+1 −1 ast_factory.hpp
+0 −1 ast_fwd_decl.hpp
+0 −7 cencode.c
+1 −0 constants.cpp
+1 −0 constants.hpp
+92 −67 context.cpp
+18 −7 context.hpp
+92 −41 eval.cpp
+3 −6 eval.hpp
+1 −1 expand.cpp
+21 −8 functions.cpp
+4 −6 functions.hpp
+14 −15 inspect.cpp
+1 −2 inspect.hpp
+0 −2 operation.hpp
+3 −3 output_nested.cpp
+103 −47 parser.cpp
+6 −3 parser.hpp
+6 −2 prelexer.cpp
+1 −0 prelexer.hpp
+13 −193 sass.cpp
+14 −137 sass.h
+49 −10 sass2scss.cpp
+646 −0 sass_context.cpp
+118 −0 sass_context.h
+107 −0 sass_functions.cpp
+76 −0 sass_functions.h
+38 −51 sass_interface.cpp
+17 −20 sass_interface.h
+250 −0 sass_values.cpp
+113 −0 sass_values.h
+1 −1 source_map.cpp
+25 −23 to_c.cpp
+15 −17 to_c.hpp
+18 −8 units.cpp
+2 −3 units.hpp
+4 −0 util.cpp
+1 −0 util.hpp
6 changes: 6 additions & 0 deletions src/sass_context_wrapper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ struct sass_context_wrapper {
NanCallback* importer_callback;
};

struct import_bag {
const char* file;
void* cookie;
Sass_Import** incs;
};

struct sass_context_wrapper* sass_make_context_wrapper(void);
void sass_free_context_wrapper(struct sass_context_wrapper* ctx_w);

Expand Down

0 comments on commit cbcacc2

Please sign in to comment.