Koichi Sasada <ko1 / atdot.net> wrote:
> On 2018/05/01 17:46, Eric Wong wrote:
> >Individual patches available at:
> >https://80x24.org/spew/20180501080844.22751-2-e / 80x24.org/raw
> >https://80x24.org/spew/20180501080844.22751-3-e / 80x24.org/raw
> >https://80x24.org/spew/20180501080844.22751-4-e / 80x24.org/raw
> >https://80x24.org/spew/20180501080844.22751-5-e / 80x24.org/raw
> 
> I'm not sure how to see all of diffs in one patch. Do you have?

I fetch and run "git diff" locally which gives me many options

REMOTE=80x24
git remote add $REMOTE git://80x24.org/ruby.git
git fetch $REMOTE
git diff $OLD $NEW

$OLD and $NEW are commits which "git request-pull" outputs in my previous
emails:

	> The following changes since commit $OLD
	> 
	>   $OLD_SUBJECT
	> 
	> are available in the Git repository at:
	> 
	>   git://80x24.org/ruby.git BRANCH
	> 
	> for you to fetch changes up to $NEW

You can also:

	curl https://80x24.org/spew/20180501080844.22751-2-e / 80x24.org/raw \
             https://80x24.org/spew/20180501080844.22751-3-e / 80x24.org/raw \
             https://80x24.org/spew/20180501080844.22751-4-e / 80x24.org/raw \
             https://80x24.org/spew/20180501080844.22751-5-e / 80x24.org/raw \
            | git am

(I run scripts from my $EDITOR and mail client, of course :)

> Anyway, small comments:
> 
> > https://80x24.org/spew/20180501080844.22751-3-e / 80x24.org/raw
> 
> > +    /* TODO: should this check is_incremental_marking() ? */
> 
> Any problem to check it?

Probably no problem, old comment.  I originally only intended to
do lazy-sweep since I have not studied incremental marking,
much.

> > +rb_gc_step(const rb_execution_context_t *ec)
> 
> How about to add assertion that rb_gc_inprogress() returns true?

I don't think that's safe.  For native_sleep callers; we release
GVL after calling rb_gc_step; so sometimes rb_gc_step becomes
a no-op (because other thread took GVL and did GC).

> --- a/internal.h
> +++ b/internal.h
> @@ -1290,6 +1290,10 @@ void rb_gc_writebarrier_remember(VALUE obj);
>  void ruby_gc_set_params(int safe_level);
>  void rb_copy_wb_protected_attribute(VALUE dest, VALUE obj);
> 
> +struct rb_execution_context_struct;
> +int rb_gc_inprogress(const struct rb_execution_context_struct *);
> +int rb_gc_step(const struct rb_execution_context_struct *);
> +
> 
> How about to add them into gc.h?

Sure.

> 
> https://80x24.org/spew/20180501080844.22751-4-e / 80x24.org/raw
> 
> I have no enough knowledge to review it.
> Nobu?
> 
> https://80x24.org/spew/20180501080844.22751-5-e / 80x24.org/raw
> 
> > @@ -288,8 +294,17 @@ rb_mutex_lock(VALUE self)
> 
> I can't understand why GC at acquiring (and restarting) timing is needed.
> Why?
> 
> For other functions, I have a same question.happen.

For mutex_lock, it only does GC if it can't acquire immediately.
Since mutex_lock cannot proceed, it can probably do GC.

I release GVL at mutex_lock before GC since it needs to give
the other thread a chance to release the mutex.

One problem I have now is threads in THREAD_STOPPED_FOREVER
state cannot continuously perform GC if some other thread
is constantly making garbage and never sleeping.

      nr = 100_000
      th = Thread.new do
        File.open('/dev/urandom') do |rd|
          nr.times { rd.read(16384) }
        end
      end

      # no improvement, since it enters sleep and stays there
      th.join

      # instead, this works (but wastes battery if there's no garbage)
      true until th.join(0.01)

So maybe we add heuristics for entering sleep for methods in
thread.c and thread_sync.c and possibly continuing to schedule
threads in THREAD_STOPPED_FOREVER state to enable them to
perform cleanup.  I don't think this is urgent, and we can
ignore this case for now.

Unsubscribe: <mailto:ruby-core-request / ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>