[ruby-core:40648] [ruby-trunk - Feature #5543][Open] rb_thread_blocking_region() API is poorly designed

[ruby-core:40648] [ruby-trunk - Feature #5543][Open] rb_thread_blocking_region() API is poorly designed

[Christopher Huff]

Issue #5543 has been reported by Christopher Huff.

----------------------------------------
Feature #5543: rb_thread_blocking_region() API is poorly designed
http://redmine.ruby-lang.org/issues/5543

Author: Christopher Huff
Status: Open
Priority: Normal
Assignee: 
Category: 
Target version: 


First, rb_thread_blocking_region() requires the blocking code to be pulled out into a separate function, scattering code through the source file and giving the coder more work to do to pass information through to that function. Something like rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(), or the BLOCKING_REGION macro used to implement rb_thread_blocking_region(), would be far more convenient to use, but were apparently deprecated and are now only usable within thread.c.

Worse, the function passed to rb_thread_blocking_region() must return a Ruby VALUE, but also must execute without a VM lock. It is rather nonsensical to specify that a function return a Ruby object while forbidding it from accessing most of Ruby. It is likely the function won't touch the anything related to Ruby at all, and while you can use casting to work around it, you shouldn't have to. The main result of all this is less readable and even somewhat misleading code.


-- 
http://redmine.ruby-lang.org

[ruby-core:40657] Re: [ruby-trunk - Feature #5543][Open] rb_thread_blocking_region() API is poorly designed

[Eric Wong]

Christopher Huff <cjameshuff@gmail.com> wrote:
> First, rb_thread_blocking_region() requires the blocking code to be
> pulled out into a separate function, scattering code through the
> source file and giving the coder more work to do to pass information
> through to that function. Something like
> rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(),
> or the BLOCKING_REGION macro used to implement
> rb_thread_blocking_region(), would be far more convenient to use, but
> were apparently deprecated and are now only usable within thread.c.

rb_thread_blocking_region_begin() and rb_thread_blocking_region_end()
require malloc()/free(), so there may be a performance impact there
for some code.

The BLOCKING_REGION macros make it too easy to break ABI compatibility
and require users to rebuild 3rd-party libraries.  ABI compatibility is
apparently an important thing for users on crippled OSes that don't
include free compilers :<

> Worse, the function passed to rb_thread_blocking_region() must return
> a Ruby VALUE, but also must execute without a VM lock. It is rather
> nonsensical to specify that a function return a Ruby object while
> forbidding it from accessing most of Ruby. It is likely the function
> won't touch the anything related to Ruby at all, and while you can use
> casting to work around it, you shouldn't have to. The main result of
> all this is less readable and even somewhat misleading code.

No return type can possibly satisfy everyone who will use this function,
VALUE is probably the least bad since the object the VALUE refers to
could be created before entering the blocking region.


I had no hand/influence in the design of this API, but I feel it's the
best API given the circumstances (need for a GVL + compatibility).

[ruby-core:40764] [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[Christopher Huff]

Issue #5543 has been updated by Christopher Huff.


VALUE is actively misleading, given that a VALUE can not be constructed by the function, and the writer of the code will most likely not want to return a pre-constructed one. "void *" is the obvious choice, not carrying such an implication and making it clear that greater care is necessary when using the function...in particular, making it clear that what it returns can't be assumed to be a valid VALUE. If we're going to be stuck with an awkward API that forces us to spin off little side functions and cram data through a single input pointer and single return pointer, we should at least not have to deal with an API that lies to us.
----------------------------------------
Feature #5543: rb_thread_blocking_region() API is poorly designed
http://redmine.ruby-lang.org/issues/5543

Author: Christopher Huff
Status: Open
Priority: Normal
Assignee: 
Category: 
Target version: 


First, rb_thread_blocking_region() requires the blocking code to be pulled out into a separate function, scattering code through the source file and giving the coder more work to do to pass information through to that function. Something like rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(), or the BLOCKING_REGION macro used to implement rb_thread_blocking_region(), would be far more convenient to use, but were apparently deprecated and are now only usable within thread.c.

Worse, the function passed to rb_thread_blocking_region() must return a Ruby VALUE, but also must execute without a VM lock. It is rather nonsensical to specify that a function return a Ruby object while forbidding it from accessing most of Ruby. It is likely the function won't touch the anything related to Ruby at all, and while you can use casting to work around it, you shouldn't have to. The main result of all this is less readable and even somewhat misleading code.


-- 
http://redmine.ruby-lang.org

[ruby-core:41902] [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[Mike Dalessio]

Issue #5543 has been updated by Mike Dalessio.



Another issue with the current design is that it does not support
callback-oriented APIs.

First, a counter-example. The current API perfectly supports the
following pattern:

  1) unlock GVL
  2) perform an expensive (or blocking) operation
  3) re-lock the GVL when ready to re-enter RubyLand

For example:

  /* calling pattern that is supported by the current API */

  VALUE time_consuming_thing(void *data)
  {
    /* ... something expensive ... */
  }

  /* native extension Ruby method */
  static VALUE get_it_for_me(VALUE self, VALUE foo)
  {
    rb_thread_blocking_region(time_consuming_thing, 0, RUBY_UBF_IO, 0):
  }

*But* if I am calling an event-oriented API (for example, an
subscribable database that invokes a callback when new records are
added), then I am stuck. An example of what I want to do:

  static int read_callback(VALUE self, Event *event) {
    rb_thread_blocking_region_end();
    rb_funcall(callable, "callback", convert_to_ruby(event));
    rb_thread_blocking_region_begin();
  }

  /* native extension Ruby method */
  VALUE stream_events(VALUE self)
  {
    rb_thread_blocking_region_begin();
    db->read(read_callback, self); /* this call may block for minutes, and may call read_callback multiple times */
    rb_thread_blocking_region_end();
  }

It appears that the current implementation (1.9.3) of
rb_thread_blocking_region_begin() and _end() does in fact support this
calling style, but they are not global symbols, and so I cannot call
them from my C extension.

----------------------------------------
Feature #5543: rb_thread_blocking_region() API is poorly designed
https://bugs.ruby-lang.org/issues/5543

Author: Christopher Huff
Status: Open
Priority: Normal
Assignee: 
Category: 
Target version: 


First, rb_thread_blocking_region() requires the blocking code to be pulled out into a separate function, scattering code through the source file and giving the coder more work to do to pass information through to that function. Something like rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(), or the BLOCKING_REGION macro used to implement rb_thread_blocking_region(), would be far more convenient to use, but were apparently deprecated and are now only usable within thread.c.

Worse, the function passed to rb_thread_blocking_region() must return a Ruby VALUE, but also must execute without a VM lock. It is rather nonsensical to specify that a function return a Ruby object while forbidding it from accessing most of Ruby. It is likely the function won't touch the anything related to Ruby at all, and while you can use casting to work around it, you shouldn't have to. The main result of all this is less readable and even somewhat misleading code.


-- 
http://redmine.ruby-lang.org

[ruby-core:41905] Re: [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[Eric Wong]

Mike Dalessio <mike.dalessio@gmail.com> wrote:
> It appears that the current implementation (1.9.3) of
> rb_thread_blocking_region_begin() and _end() does in fact support this
> calling style, but they are not global symbols, and so I cannot call
> them from my C extension.

rb_thread_call_with_gvl() is globally-visible (but not in headers)
for 1.9.3: https://bugs.ruby-lang.org/issues/4328

Perhaps you can use that?

[ruby-core:41933] [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[Mike Dalessio]

Issue #5543 has been updated by Mike Dalessio.


> rb_thread_call_with_gvl() is globally-visible (but not in headers)
> for 1.9.3: https://bugs.ruby-lang.org/issues/4328
> 
> Perhaps you can use that?

That is *exactly* what I need.

Thanks so much for the advice. You made my day! <3 <3 <3

----------------------------------------
Feature #5543: rb_thread_blocking_region() API is poorly designed
https://bugs.ruby-lang.org/issues/5543

Author: Christopher Huff
Status: Open
Priority: Normal
Assignee: 
Category: 
Target version: 


First, rb_thread_blocking_region() requires the blocking code to be pulled out into a separate function, scattering code through the source file and giving the coder more work to do to pass information through to that function. Something like rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(), or the BLOCKING_REGION macro used to implement rb_thread_blocking_region(), would be far more convenient to use, but were apparently deprecated and are now only usable within thread.c.

Worse, the function passed to rb_thread_blocking_region() must return a Ruby VALUE, but also must execute without a VM lock. It is rather nonsensical to specify that a function return a Ruby object while forbidding it from accessing most of Ruby. It is likely the function won't touch the anything related to Ruby at all, and while you can use casting to work around it, you shouldn't have to. The main result of all this is less readable and even somewhat misleading code.


-- 
http://redmine.ruby-lang.org

[ruby-core:41935] [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[Eric Hodel]

Issue #5543 has been updated by Eric Hodel.


I'm using rb_thread_call_with_gvl as well to support GLUT (OpenGL toolkit) callbacks.

I'd like something like this to be officially supported, not just accessible.
----------------------------------------
Feature #5543: rb_thread_blocking_region() API is poorly designed
https://bugs.ruby-lang.org/issues/5543

Author: Christopher Huff
Status: Open
Priority: Normal
Assignee: 
Category: 
Target version: 


First, rb_thread_blocking_region() requires the blocking code to be pulled out into a separate function, scattering code through the source file and giving the coder more work to do to pass information through to that function. Something like rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(), or the BLOCKING_REGION macro used to implement rb_thread_blocking_region(), would be far more convenient to use, but were apparently deprecated and are now only usable within thread.c.

Worse, the function passed to rb_thread_blocking_region() must return a Ruby VALUE, but also must execute without a VM lock. It is rather nonsensical to specify that a function return a Ruby object while forbidding it from accessing most of Ruby. It is likely the function won't touch the anything related to Ruby at all, and while you can use casting to work around it, you shouldn't have to. The main result of all this is less readable and even somewhat misleading code.


-- 
http://redmine.ruby-lang.org

[ruby-core:42897] [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[Koichi Sasada]

Issue #5543 has been updated by Koichi Sasada.

Assignee set to Koichi Sasada
Target version set to 2.0.0

Christopher Huff wrote:
> VALUE is actively misleading, given that a VALUE can not be constructed by the function, and the writer of the code will most likely not want to return a pre-constructed one. "void *" is the obvious choice, not carrying such an implication and making it clear that greater care is necessary when using the function...in particular, making it clear that what it returns can't be assumed to be a valid VALUE. If we're going to be stuck with an awkward API that forces us to spin off little side functions and cram data through a single input pointer and single return pointer, we should at least not have to deal with an API that lies to us.

Should we change it from VALUE to 'void *'?
It's not big compatibility issue, I think (maybe it causes several warnings at compiling time).



----------------------------------------
Feature #5543: rb_thread_blocking_region() API is poorly designed
https://bugs.ruby-lang.org/issues/5543

Author: Christopher Huff
Status: Open
Priority: Normal
Assignee: Koichi Sasada
Category: 
Target version: 2.0.0


First, rb_thread_blocking_region() requires the blocking code to be pulled out into a separate function, scattering code through the source file and giving the coder more work to do to pass information through to that function. Something like rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(), or the BLOCKING_REGION macro used to implement rb_thread_blocking_region(), would be far more convenient to use, but were apparently deprecated and are now only usable within thread.c.

Worse, the function passed to rb_thread_blocking_region() must return a Ruby VALUE, but also must execute without a VM lock. It is rather nonsensical to specify that a function return a Ruby object while forbidding it from accessing most of Ruby. It is likely the function won't touch the anything related to Ruby at all, and while you can use casting to work around it, you shouldn't have to. The main result of all this is less readable and even somewhat misleading code.


-- 
http://bugs.ruby-lang.org/

[ruby-core:45856] [ruby-trunk - Feature #5543][Feedback] rb_thread_blocking_region() API is poorly designed

[ko1 (Koichi Sasada)]

Issue #5543 has been updated by ko1 (Koichi Sasada).

Status changed from Assigned to Feedback

ping: cjameshuff

----------------------------------------
Feature #5543: rb_thread_blocking_region() API is poorly designed
https://bugs.ruby-lang.org/issues/5543#change-27443

Author: cjameshuff (Christopher Huff)
Status: Feedback
Priority: Normal
Assignee: ko1 (Koichi Sasada)
Category: 
Target version: 2.0.0


First, rb_thread_blocking_region() requires the blocking code to be pulled out into a separate function, scattering code through the source file and giving the coder more work to do to pass information through to that function. Something like rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(), or the BLOCKING_REGION macro used to implement rb_thread_blocking_region(), would be far more convenient to use, but were apparently deprecated and are now only usable within thread.c.

Worse, the function passed to rb_thread_blocking_region() must return a Ruby VALUE, but also must execute without a VM lock. It is rather nonsensical to specify that a function return a Ruby object while forbidding it from accessing most of Ruby. It is likely the function won't touch the anything related to Ruby at all, and while you can use casting to work around it, you shouldn't have to. The main result of all this is less readable and even somewhat misleading code.


-- 
http://bugs.ruby-lang.org/

[ruby-core:46137] [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[larskanis1 (Lars Kanis)]

Issue #5543 has been updated by larskanis1 (Lars Kanis).


ko1 (Koichi Sasada) wrote:
> Should we change it from VALUE to 'void *'?

IMHO VALUE is misleading and should be changed for rb_thread_blocking_region() and rb_blocking_function_t. But isn't it better to use "int" instead of "void*" ? Where should the "void*" point to? For data1/data2 a "void*" is obviously the right choice to pass a locally defined struct  with parameters to (or back from) the blocking function. But the return should be a copied value instead of a pointer. The same applies to rb_thread_call_with_gvl().

When I used rb_thread_blocking_region(), I was wondering about the return type VALUE, too. So for instance in the pkcs11.gem the return value is passed via a struct through *data1 to avoid the use of misleading VALUE at all.

----------------------------------------
Feature #5543: rb_thread_blocking_region() API is poorly designed
https://bugs.ruby-lang.org/issues/5543#change-27749

Author: cjameshuff (Christopher Huff)
Status: Feedback
Priority: Normal
Assignee: ko1 (Koichi Sasada)
Category: 
Target version: 2.0.0


First, rb_thread_blocking_region() requires the blocking code to be pulled out into a separate function, scattering code through the source file and giving the coder more work to do to pass information through to that function. Something like rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(), or the BLOCKING_REGION macro used to implement rb_thread_blocking_region(), would be far more convenient to use, but were apparently deprecated and are now only usable within thread.c.

Worse, the function passed to rb_thread_blocking_region() must return a Ruby VALUE, but also must execute without a VM lock. It is rather nonsensical to specify that a function return a Ruby object while forbidding it from accessing most of Ruby. It is likely the function won't touch the anything related to Ruby at all, and while you can use casting to work around it, you shouldn't have to. The main result of all this is less readable and even somewhat misleading code.


-- 
http://bugs.ruby-lang.org/

[ruby-core:46138] [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[nobu (Nobuyoshi Nakada)]

Issue #5543 has been updated by nobu (Nobuyoshi Nakada).


"int" may not be large enough to keep a pointer.
----------------------------------------
Feature #5543: rb_thread_blocking_region() API is poorly designed
https://bugs.ruby-lang.org/issues/5543#change-27750

Author: cjameshuff (Christopher Huff)
Status: Feedback
Priority: Normal
Assignee: ko1 (Koichi Sasada)
Category: 
Target version: 2.0.0


First, rb_thread_blocking_region() requires the blocking code to be pulled out into a separate function, scattering code through the source file and giving the coder more work to do to pass information through to that function. Something like rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(), or the BLOCKING_REGION macro used to implement rb_thread_blocking_region(), would be far more convenient to use, but were apparently deprecated and are now only usable within thread.c.

Worse, the function passed to rb_thread_blocking_region() must return a Ruby VALUE, but also must execute without a VM lock. It is rather nonsensical to specify that a function return a Ruby object while forbidding it from accessing most of Ruby. It is likely the function won't touch the anything related to Ruby at all, and while you can use casting to work around it, you shouldn't have to. The main result of all this is less readable and even somewhat misleading code.


-- 
http://bugs.ruby-lang.org/

[ruby-core:46140] Re: [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[SASADA Koichi]

(2012/07/03 18:41), nobu (Nobuyoshi Nakada) wrote:
> "int" may not be large enough to keep a pointer.

+1.

-- 
// SASADA Koichi at atdot dot net

[ruby-core:46239] [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[larskanis1 (Lars Kanis)]

Issue #5543 has been updated by larskanis1 (Lars Kanis).


OK, then use 'void *' like rb_thread_call_with_gvl().
----------------------------------------
Feature #5543: rb_thread_blocking_region() API is poorly designed
https://bugs.ruby-lang.org/issues/5543#change-27866

Author: cjameshuff (Christopher Huff)
Status: Feedback
Priority: Normal
Assignee: ko1 (Koichi Sasada)
Category: 
Target version: 2.0.0


First, rb_thread_blocking_region() requires the blocking code to be pulled out into a separate function, scattering code through the source file and giving the coder more work to do to pass information through to that function. Something like rb_thread_blocking_region_begin() and rb_thread_blocking_region_end(), or the BLOCKING_REGION macro used to implement rb_thread_blocking_region(), would be far more convenient to use, but were apparently deprecated and are now only usable within thread.c.

Worse, the function passed to rb_thread_blocking_region() must return a Ruby VALUE, but also must execute without a VM lock. It is rather nonsensical to specify that a function return a Ruby object while forbidding it from accessing most of Ruby. It is likely the function won't touch the anything related to Ruby at all, and while you can use casting to work around it, you shouldn't have to. The main result of all this is less readable and even somewhat misleading code.


-- 
http://bugs.ruby-lang.org/

[ruby-core:46295] Re: [ruby-trunk - Feature #5543] rb_thread_blocking_region() API is poorly designed

[SASADA Koichi]

(2012/07/08 5:06), larskanis1 (Lars Kanis) wrote:
> 
> OK, then use 'void *' like rb_thread_call_with_gvl().

We conclude this feature follows:

(1) Don't touch the declaration of rb_thread_blocking_region().
    And mark it as obsolete.

(the name "blocking_region" is internal name.  I think it was bad name
to expose.

  BLOCKING_REGION(
    // from here
    do something without gvl
    // to here
  )
)

Do not use this API for newer extensions.  And replace it with the
call_without_gvl if you can.


(2) Change the return type rb_thread_call_without_gvl()

void *
rb_thread_call_without_gvl(
    void *(*func)(void *), void *data1,
    rb_unblock_function_t *ubf, void *data2)

rb_thread_call_without_gvl() and rb_thread_call_with_gvl() is
experimental (not exposed officially) function.  So we can expect that
only a few "compiling error" with it.

-- 
// SASADA Koichi at atdot dot net