Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace odd -j in modules-submake with ability for proper parallelism. #128

Closed

Conversation

wdoekes
Copy link
Contributor

@wdoekes wdoekes commented Nov 14, 2013

Recently -j was added to the list of flags passed to the modules
$(MAKE) command. That's bad practise for two reasons: (1) the caller
should decide on how much -j is required, not the programmer, and (2)
just -j without any numeric argument is "use all resources you can
get", which can be too much.

We order the Makefile around so the supermake (the user invocation)
can dispatch individual submakes in a parallel fashion. This is done
by (a) making the module directories phony targets and (b) replacing
the sequential for-loop over the modules with a simple dependency.

And, to top it off, I ran a quick and dirty non-scientific test (*) and came up with these tests results:

Before patch, make modules -j3:

real 1m23.744s
user 2m24.796s
sys 0m7.252s

After patch, make modules -j3:

real 1m2.799s
user 2m51.112s
sys 0m7.400s

That's quite a bit of speedup too. Even though that wasn't the goal.

(*) I ran the test twice to rule out caching issues in favor of either one of them and came up with pretty much the same numbers both times.

Recently `-j` was added to the list of flags passed to the modules
$(MAKE) command. That's bad practise for two reasons: (1) the caller
should decide on how much -j is required, not the programmer, and (2)
just -j without any numeric argument is "use all resources you can
get", which can be too much.

We order the Makefile around so the supermake (the user invocation)
can dispatch individual submakes in a parallel fashion. This is done
by (a) making the module directories phony targets and (b) replacing
the sequential for-loop over the modules with a simple dependency.
@wdoekes
Copy link
Contributor Author

wdoekes commented Nov 14, 2013

Sorry for the duplicate. My first pull request was off master. I now force-updated the repo and moved that patch to this branch.

Bogdan already commented. He said:


Hi Walter,

I agree with you said, nevertheless compiling all modules in the same time may be scary and unmanageable - you get the compiling logs mixed all together (from all modules) and in case of compile failure , hard to figure out what module the file belongs to :(...

I would suggest a default "-j3" which can be override by user and a super-fast compile mode based on your patch (by default off) triggered via an extra switch or so....

@wdoekes
Copy link
Contributor Author

wdoekes commented Nov 20, 2013

Ok. So I took your concerns, and did the following:

  • When building modules in parallel, trap the error and display a notice that things went wrong:

    Compiling dbt_tb.c
    Compiling dbt_util.c
    Linking db_text.so
    make[2]: Leaving directory `/home/walter/src/opensips-wjd/modules/db_text'
    Linking db_virtual.so
    make[2]: Leaving directory `/home/walter/src/opensips-wjd/modules/db_virtual'
    make[1]: Leaving directory `/home/walter/src/opensips-wjd'
    
    Building one or more modules failed!
    Please re-run make without -j / --jobs to find out which.
    
    make: *** [modules] Error 2
    make: *** Waiting for unfinished jobs....
    
  • I re-added the linefeeds when doing a build without -j:

    make -C modules/closeddial
    make[2]: Entering directory `/home/walter/src/opensips-wjd/modules/closeddial'
    make[2]: `closeddial.so' is up to date.
    make[2]: Leaving directory `/home/walter/src/opensips-wjd/modules/closeddial'
    
    
    make -C modules/db_cachedb
    make[2]: Entering directory `/home/walter/src/opensips-wjd/modules/db_cachedb'
    Compiling dbase.c
    dbase.c:36:7: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘struct’
    dbase.c: In function ‘db_cachedb_init’:
    dbase.c:64:10: error: ‘db_cachedb_script_urls’ undeclared (first use in this function)
    dbase.c:64:10: note: each undeclared identifier is reported only once for each function it appears in
    make[2]: *** [dbase.o] Error 1
    make[2]: Leaving directory `/home/walter/src/opensips-wjd/modules/db_cachedb'
    make[1]: *** [modules/db_cachedb] Error 2
    make[1]: Leaving directory `/home/walter/src/opensips-wjd'
    make: *** [modules] Error 2
    
  • I still feel that the user shouldn't get any -j unless he explicitly requests so, so I left that as it was.

    By the way:

    When the system is heavily loaded, you will probably want to run fewer jobs than when it is lightly loaded. You can use the ‘-l’ option to tell make to limit the number of jobs to run at once, based on the load average. The ‘-l’ or ‘--max-load’ option is followed by a floating-point number. For example,
    -l 2.5
    will not let make start more than one job if the load average is above 2.5. The ‘-l’ option with no following number removes the load limit, if one was given with a previous ‘-l’ option.

    Apparently -l was the option to limit jobs if -j doesn't have an argument.

Non-scientific tests says, before:

$ touch Makefile; time make -j4 all
real    2m22.007s
user    3m22.776s
sys 0m14.944s

.. and after:

$ touch Makefile; time make -j4 all
real    2m4.211s
user    3m29.292s
sys 0m14.960s

@razvancrainea razvancrainea self-assigned this Mar 18, 2014
@razvancrainea
Copy link
Member

I added a new Makefile switch, called FASTER, that acts as a switch for this commit. For more information, see: http://www.opensips.org/Documentation/Install-CompileAndInstall-1-11#toc6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants