From: Eric Wong Date: 2017-06-12T21:48:12+00:00 Subject: [ruby-core:81653] Re: [Ruby trunk Feature#12589] VM performance improvement proposal Eric Wong wrote: > vmakarov@redhat.com 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/20170606012921.26806-1-e@80x24.org/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: