[ruby-core:81653] Re: [Ruby trunk Feature#12589] VM performance improvement proposal

Eric Wong <[e-mail]> wrote:
> [e-mail] wrote:
> > I should remove -Werror=incompatible-pointer-types from the script and
> > restrict added by me.  They are not important.
> 
> Actually, I've discovered AC_C_RESTRICT is convenient to add to
> configure.in and I would like us to be able to take advantage of
> useful C99 (and C1x) features as they become available:
> 
>    https://80x24.org/spew/[e-mail]/raw

Ah, I noticed you've removed "restrict" from your branch.
Technically, wouldn't that be a regression from an optimization
standpoint? (of course you know far more about compiler
optimization than I).

> Perhaps -Werror=incompatible-pointer-types can be made a
> standard warning flag for building Ruby, too...

That removal was fine by me.

Not a particularly focused review, just random stuff I'm
spotting while taking breaks from other projects.

Mostly just mundane systems stuff, nothing about the actual
mjit changes.

* I noticed mjit.c uses it's own custom doubly-linked list for
  rb_mjit_batch_list.  For me, that places a little extra burden
  in having extra code to review.  Any particular reason ccan/list
  isn't used?

  Fwiw, the doubly linked list implementation in compile.c
  predated ccan/list; and I didn't want to:

    a) risk throwing away known-working code

    b) introduce a the teeny performance regression for loop-heavy
       code:

       ccan/list is faster for insert/delete, but slightly
       slower iteration for loops from what I could tell.


* The pthread_* stuff can probably use portable stuff defined in
  thread.c and thread_*.h.  (Unfortunately for me) Ruby needs to
  support non-Free platforms :<


* fopen should probably be replaced by something which sets
  cloexec; since the "e" flag of fopen is non-portable.

  Perhaps rb_cloexec_open() + fdopen().


* It looks like meant to use fflush instead of fsync; fflush is
  all that's needed to ensure other processes can see the file
  changes (and it's done transparently by fclose).  fsync is to
  ensure the file is committed to stable storage, and some folks
  still use stable storage for /tmp.  fsync before the final
  fflush is wrong, even, as the kernel may not have all the
  data from userspace


* get_uniq_fname should respect alternate tmpdirs like Dir.tmpdir, does
  (in lib/dir.rb)


* we can use vfork + execve instead of fork to speed up process
  creation; just need to move the fopen (which can call malloc)
  into the parent.  We've already used vfork for Process.spawn,
  system(), ``, IO.popen for a few years.


None of these are super important; and I can eventually take
take some time to make send you patches or pull requests (via
email/redmine)

rb_mjit_min_header-2.5.0.h takes forever to build...

Thank again for taking your time to work on Ruby!

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