Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)

2018-05-29 Thread Kirill Kolyshkin
@ffesti @pmatilai please see the updated (and rebased) patch set. > rpm comment style is "/* foo */", "// foo" should not be used I actually thought about it so I did `git grep '// '` which revealed quite a few uses and so I went ahead. Anyway, done. > and put the prototype to rpmio_internal.h

Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)

2018-05-21 Thread Kirill Kolyshkin
@pmatilai @ffesti PTAL -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/444#issuecomment-390716012___ Rpm-maint mailing list Rpm-m

Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)

2018-05-17 Thread Kirill Kolyshkin
> Filed https://bugzilla.gnome.org/show_bug.cgi?id=796227 This one is bad, as ulimit might have been changed during the runtime of a process, and so using rlim_max is more correct and safe. For the same reason I have added the second commit https://github.com/rpm-software-management/rpm/pull/44

Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)

2018-05-17 Thread Kirill Kolyshkin
> One other thing is, they call getrlimit(RLIMIT_NOFILE, &rl) and use > rl.rlim_max which seems to be a mistake -- rlim_cur should be used. Filed https://bugzilla.gnome.org/show_bug.cgi?id=796227 -- You are receiving this because you are subscribed to this thread. Reply to this email directly o

Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)

2018-05-17 Thread Kirill Kolyshkin
@cgwalters I took a look at glib implementation, they make use of `fdwalk()` if available (it's a function unique to SunOS/Solaris AFAICS), and reimplement it using /proc/self/fd if not (which is the case for Linux). This seems to be an unnecessary complication (unless we care much about Solari

Re: [Rpm-maint] [rpm-software-management/rpm] Optimize and unite setting CLOEXEC on fds (#444)

2018-05-17 Thread Kirill Kolyshkin
OK, patch updated to take all the review comments into account (thanks @Saur2000 for your suggestions) so it's now also fixing the `doScriptExec()` from `lib/rpmscript.c`. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: htt

Re: [Rpm-maint] [rpm-software-management/rpm] luaext/Pexec: optimize setting CLOEXEC (#444)

2018-05-17 Thread Kirill Kolyshkin
For the reference, here's my quick-n-dirty benchmarking code: ```c #include #include #include #include #include #include static void set_cloexec(int fd) { int flags = fcntl(fd, F_GETFD); if (flags == -1 || (flags & FD_CLOEXEC)) return; fcntl(fd, F_

Re: [Rpm-maint] [rpm-software-management/rpm] luaext/Pexec: optimize setting CLOEXEC (#444)

2018-05-17 Thread Kirill Kolyshkin
``` kir@kd:~/git/rpm$ time ./a 64 100 p real0m3.413s user0m0.416s sys 0m2.994s kir@kd:~/git/rpm$ time ./a 64 100 i real0m3.661s user0m1.421s sys 0m2.233s ``` -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view

Re: [Rpm-maint] [rpm-software-management/rpm] luaext/Pexec: optimize setting CLOEXEC (#444)

2018-05-17 Thread Kirill Kolyshkin
I did some more benchmarks comparing the new /proc way with the old one. It seems that the old one is faster when `open_max < 60` which should almost never be the case. Based on that and the suggestion from @Saur2000 I'm dropping the conditional switch to old method completely. -- You are rece

Re: [Rpm-maint] [rpm-software-management/rpm] luaext/Pexec: optimize setting CLOEXEC (#444)

2018-05-17 Thread Kirill Kolyshkin
kolyshkin commented on this pull request. > + fcntl(fd, F_SETFD, FD_CLOEXEC); +} + +static void set_cloexec_all(void) +{ + const int min_fd = 3; // don't touch stdin/out/err + const int default_open_max = 1024; + int fd, open_max; + + open_max = sysconf(_SC_OPEN_MAX);

Re: [Rpm-maint] [rpm-software-management/rpm] luaext/Pexec: optimize setting CLOEXEC (#444)

2018-05-17 Thread Kirill Kolyshkin
kolyshkin commented on this pull request. > @@ -330,26 +330,64 @@ static int Pmkfifo(lua_State *L)/** > mkfifo(path) */ } +static void set_cloexec(int fd) +{ + int flag = fcntl(fd, F_GETFD); + + if (flag == -1 || (flag & FD_CLOEXEC)) + return; + +

Re: [Rpm-maint] [rpm-software-management/rpm] luaext/Pexec: optimize setting CLOEXEC (#444)

2018-05-17 Thread Kirill Kolyshkin
kolyshkin commented on this pull request. > + struct dirent *entry; + while ((entry = readdir(dir)) != NULL) { + fd = atoi(entry->d_name); + if (fd >= min_fd) + set_cloexec(fd); + } + closedir(dir); + + return; + +fall

Re: [Rpm-maint] [rpm-software-management/rpm] luaext/Pexec: optimize setting CLOEXEC (#444)

2018-05-17 Thread Kirill Kolyshkin
The CI failure on rawhide is unrelated, it's a dnf error complaining about repo metadata: > raise ValueError("The supplied metadata version isn't supported") -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://gith

[Rpm-maint] [rpm-software-management/rpm] luaext/Pexec: optimize setting CLOEXEC (#444)

2018-05-17 Thread Kirill Kolyshkin
In case maximum number of open files limit is set too high, Pexec() spends way too much time trying to set FD_CLOEXEC for all those file descriptors, resulting in severe increase of time it takes to execute rpm or dnf. For example, this happens while running under Docker because: > $ docker run

Re: [Rpm-maint] [rpm-software-management/rpm] Missing build id, Generating build-id links failed (#367)

2017-12-08 Thread Kirill Kolyshkin
OK one way is to use `gccgo` instead of `go build`, and the other way is to add `-ldflags=-linkmode=external` flag to `go build`. Still, by default (i.e. using `go build`) it doesn't do that and it's a problem. -- You are receiving this because you commented. Reply to this email directly or vi

Re: [Rpm-maint] [rpm-software-management/rpm] Missing build id, Generating build-id links failed (#367)

2017-12-08 Thread Kirill Kolyshkin
Here is a simple reproducer for the case, on a clean fresh Fedora 27 install: ```spec # Name this gotst.spec Name: gotst Version:1.0 Release:1%{?dist} Summary:Go binary to demonstrate build-id problem Group: Applications License:GPL BuildRequire