Issue754

Title Remove options to build in 32-bit/64-bit mode; use machine/user defaults instead
Priority feature Status resolved
Superseder Nosy List atorralba, florian, gabi, guillem, jendrik, malte, silvan
Assigned To florian Keywords
Optional summary

Created on 2017-12-01.10:27:16 by jendrik, last changed by malte.

Messages
msg8485 (view) Author: malte Date: 2019-01-21.12:22:56
Great! :-)
msg8472 (view) Author: florian Date: 2019-01-18.17:08:53
Jendrik and I merged the new buildbot worker (infrastructure repo) and the
changes to the build system (Fast Downward repo) today. We also updated the
documentation in the wiki.
msg8359 (view) Author: malte Date: 2018-12-15.16:42:43
I renamed the issue title to reflect the discussion of the last few days.
msg8353 (view) Author: florian Date: 2018-12-14.17:05:36
I copied the Dockerfile for our bionic worker, changed the source of images from
"ubuntu" to "i386/ubuntu" and removed some parts related to CPLEX and autodoc.
The resulting image worked out of the box and successfully compiled a 32-bit
planner when running "./build.py releasenative". 

I also added the worker to docker-compose and secret_passwords so the buildbot
should use it when we push the changes there. We can only test this after
merging this issue, though. The changes are in the infrastructure repo.
msg8349 (view) Author: malte Date: 2018-12-14.15:52:19
Vanilla Ubuntu 18.04 is 64-bit only, but there seem to be 32-bit docker images.
Still, I'm not sure how easy it will be to set up our dependencies for this
platform. If it's easy, I think the plan makes sense, but otherwise it might be
a good idea to go with something that officially supports 32-bit, like Debian or
Ubuntu 16.04.
msg8347 (view) Author: florian Date: 2018-12-14.15:42:59
I agree and will add it to the branch in the infrastructure repo. I'll stick
with one worker running Ubuntu 18.04, g++-8 and no LP solver for now. Let me
know if I should add more.
msg8346 (view) Author: malte Date: 2018-12-14.15:35:54
I think a docker image with a 32-bit Ubuntu would make more sense than using
multilib. As I said previously, it makes sense for the docker systems to mimic
realistic user scenarios as closely as possible, and the realistic reason for
wanting a 32-bit build is because you're on a 32-bit system.
msg8345 (view) Author: florian Date: 2018-12-14.15:32:08
I think the easiest and most natural way to test a 32-bit Linux build is to add
a worker for this and set it up so it builds 32-bit binaries (as opposed to the
way we implemented it previously which ran an additional build on the existing
worker).

We can either use the exsting xenial and/or bionic images and install multilib
on them or use a new Docker image based on a 32-bit Ubuntu
(https://hub.docker.com/r/i386/ubuntu/). Using existing Docker files, we'd only
need changes in the docker-compose file, with the new Docker image, we 
additionally would need to add another Dockerfile.
msg8342 (view) Author: malte Date: 2018-12-14.15:05:10
Jendrik is right that the issue title is misleading, but I think that's because
the title doesn't reflect our current state of discussion. We changed the title
of issue213 for the same reason. See msg7425 at issue213 for some background.

The reason for preferring native builds is that forcing any kind of special
build complicates the build system and imposes unusual dependencies (like the
"multilib" packages and special flags that must be passed to the OSI library's
configure scripts). This is true for forcing 64-bit builds as much as it is for
forcing 32-bit builds.

Practically speaking, most desktop OSes are 64-bit now. Mac OS is moving to
64-bit only, and this year (or perhaps already in 17.10) Ubuntu dropped 32-bit
OSes in their new releases. (But existing 32-bit Ubuntus will keep being
supported until 2023 or so.) According to some fairly recent data I found, 98%
of Windows steam users run a 64-bit Windows, although gamers of course tend to
have more modern hardware than the average. So 32-bit is definitely on the way
out for desktops.

But I think there is still value in making sure 32-bit builds work, unless they
start holding us back in any way, for which we currently have no evidence. For
example, if we want planners to be easily usable in a robotics context, it makes
a lot of sense to support more restricted hardware if it costs us nothing. (If
there were any 64-bit-only features that would be useful to us, that might
change the story.)
msg8341 (view) Author: florian Date: 2018-12-14.14:46:59
We are not keeping the 32-bit build option. After merging this issue, we always
make native builds. As Malte said: the idea is that the user decides the build
environment by setting the correct environment variables and installing the
right library versions. The CMake files do not force either 32-bit or 64-bit.
msg8340 (view) Author: jendrik Date: 2018-12-14.14:41:03
I am surprised that we're now discussing whether we want to keep the 32-bit
build option, given the title of the issue and msg6675.
msg8332 (view) Author: malte Date: 2018-12-14.13:56:15
Makes sense, but then I suppose we should also test a 32-bit Linux build. If we
do something that breaks the 32-bit compile, I don't think we want to be forced
to debug it on Windows.

The basic idea we want to follow now is that the Linux CMakefiles are agnostic
to the build environment and the user can control their build with the usual
options like CCOPT etc.

It would make sense to test that, first locally and ideally later also in a
buildbot build. This may also be useful to test for the "get rid of the -Werror
default" issue.
msg8331 (view) Author: florian Date: 2018-12-14.13:52:03
Removing the 32-bit Windows build would only save us 4 lines in the buildbot
config. I think it is worth keeping it, in particular because it is our only
32-bit test.

So far, I thought we didn't plan to drop support for 32-bit, i.e., 32-bit builds
would still be supported in a 32-bit environment. This issue is only about
changing the default and leaving the choice up to the environment instead of
forcing it in CMake.

So if we still want to support 32-bit builds I think it makes sense to test
them, so we don't accidentally introduce changes that break the 32-bit build. In
the Windows build testing the 32-bit build is easily possible because we have to
specify the bitwidth anyway. 

If we want to drop 32-bit support I would prefer this to be an active decision
and maybe add warnings if the planner is used in 32-bit mode.
msg8326 (view) Author: jendrik Date: 2018-12-14.12:16:22
I don't see a reason why we should keep the 32-bit option for Windows.
msg8325 (view) Author: silvan Date: 2018-12-14.12:13:30
I had a look at the pull request and found it to be good :-) I also looked at
the ai-infrastructure diff (didn't know Malte was doing so, too) and found that
two build steps were missing in master.cfg, which I added. I think indeed that
we should remove the -m64 bit flags from the workers. 

If we can easily drop the support for 32bit builds for the Windows worker, too,
I would also be in favor of doing so, assuming that our Windows machine uses a
64bit OS.
msg8324 (view) Author: malte Date: 2018-12-14.12:01:55
I have had a look at the ai-infrastructure diff. Shouldn't we remove the
explicit "-m64" options in the configure calls in the Dockefiles for the xenial
and bionic workers? I'm also not sure if we should continue passing --enable-static.

Regarding the Windows question, I'm not sure if we should continue to support a
32-bit Windows build if CPLEX doesn't work with this. If it doesn't make
anything more complicated, I'm not against it, but only then.
msg8319 (view) Author: florian Date: 2018-12-14.11:38:38
I added a branch in the infrastructure repository that uses the new build
options and sets the workers up correctly (changing names of environment
variables, not installing multilib).

On Windows, we have to specify the bit width when loading the compiler
environment, and I kept both options (32-bit and 64-bit build). On all other
workers, we now only test a (native) 64-bit build.

If someone want to review the change, you can find it in the repository
ai/infrastructure by comparing the default branch to the branch remove32bit. The
branch assumes the changes in the Fast Downward repository:
https://bitbucket.org/FlorianPommerening/downward-issues/pull-requests/47

I merged default into the branch of Fast Downward after Malte reviewed it. Since
there were conflicts, it might be good to do a light review of these changes as
well.
msg7429 (view) Author: malte Date: 2018-09-13.12:23:08
Great! Looks good to merge when the other issues are ready.
msg7428 (view) Author: jendrik Date: 2018-09-13.12:16:31
The make-ipc-submission script works now.
msg7426 (view) Author: jendrik Date: 2018-09-13.12:07:03
I'll take care of the comment.
msg7423 (view) Author: malte Date: 2018-09-13.11:52:36
I left one comment on bitbucket. Looks good. :-)
msg7369 (view) Author: gabi Date: 2018-09-11.15:43:21
Silvan and I tested this with our existing setups, in particular builds on
Windows, Mac and Linux. We did not find a 32-bit linux or Mac system. We also
could not test the LP support on Mac OS (not available on the build bot) and on
Windows. For windows, we first need to install a CPLEX version that works with
our current version of Visual studio but this is a separate issue where we will
ask Augusto for help (also upgrading Visual Studio).

For some unclear reason, we had to use slashes in the environment variables for
coin in Windows.

A test run of autodoc ran through smoothly.

From my point of view, this can be merged as soon as the rest of issue213 is
resolved.

Pull request at
https://bitbucket.org/FlorianPommerening/downward-issues/pull-requests/47/issue754
msg7359 (view) Author: florian Date: 2018-09-10.17:44:50
Gabi and I created a pull request. We should wait with merging it until issue213
is merged and there are a couple of other things we need to test and update as
well (wiki, autodoc, buildbot, code tests).

One place where we left the distinction between 32 and 64 bit is in the find
script for CPLEX. The reason for that is that the default installation of CPLEX
has different paths on different systems and we need to know the bitwidth, OS,
and release/debug mode to set the correct hint path. This is not a build option
that the user could set, i.e., we do not support cross compilation, it merely is
a more generic find script that supports native builds on more systems.
msg6680 (view) Author: jendrik Date: 2017-12-01.12:56:11
I completely agree, Malte. Sorry for the confusion.
msg6676 (view) Author: malte Date: 2017-12-01.12:03:27
> that broke the camel's necks.

Or its back, rather. I'm sure most camels only have one neck, and that the neck
is not generally overloaded by straw.
msg6675 (view) Author: malte Date: 2017-12-01.12:00:56
Jendrik, we have long decided that the 32-bit build will go away. It is just a
question of when. Indefinite deprecation is not an option. We are happy to hear
everyone's concerns about this decision and discuss how to address them, but
this is not going to change the decision.

The most critical reason why we made this decision was that 32-bit CPLEX has
been discontinued, and it is not really possible to continue using the old
versions unless you already happen to have a working old version of the code
lying around on some machines and are happy to never touch the software stack of
these machines (or move to newer machines) ever again. This is by far not the
only reason (others include the huge effort needed to support 32-bits especially
with the library dependencies and cross-platform builds and the way they
complicate the build instructions by requiring obscure cross-compiling
libraries), but it was the straw that broke the camel's necks.

I should also point out that the reason we still have a 32-bit build is not
"mainly because of the memory performance", but *only* because of memory. The
64-bit build was already faster than the 32-bit build in 2011, and the numbers
I've seen since then have agreed with this. (However, I haven't seen many
numbers, something we should perhaps address.)


Alvaro, the core data structures within Fast Downward used to heavily rely on
pointers, but we have started to replace them one by one by things that are less
wasteful with memory when compiled in 64-bit mode. More details on this are in
issue213. It's been a large effort (issue213 is now six years old, and we've
been chipping away at it for a good chunk of that time), but we're now mostly there.

One of the early examples of this was the mechanism for storing state data,
which used to be based on individual vectors of state data for each state and is
now based on large contiguous bins of packed integers. Another example of this
are the hash tables used to store the state registry, where we have to stop
using std::unordered_map and roll our own instead.

I hope that memory-efficient BDD libraries for 64-bit builds already exist,
given that 64 bits are now ubiquitous. It's been a while since I last looked
into BDD library implementations, but back then the ones I looked at didn't use
pointers internally anyway and would have been fine in 64-bit builds. If this is
no longer case, it is really up to the people providing these library
implementations to step up and modernize their code. "If you want to use our
library, don't compile a 64-bit executable" is not an acceptable stance any more
these days.

When our own changes to make the code more 64-bit friendly are done and we are
happy with the memory performance, we will remove the 32-bit options from cmake.
The removal will perhaps not happen overnight, but I don't think we will wait
for a long time before doing it. Of course it will be possible for forks of  the
code to keep these options in, and they will continue to work for the
foreseeable future if the LP code is not used.

The only scenario where I can see this decision changed is if someone interested
in championing 32-bit builds step up to take over the support of 32-bit builds.
This mainly entails taking charge of the cmake files, documentation, and 32-bit
build slaves.
msg6673 (view) Author: atorralba Date: 2017-12-01.11:13:54
My two cents on this. In my experience, compiling on 32/64 bits has the most 
impact on the performance of data-structures that heavily rely on pointers 
(e.g. BDDs). If something alike is being heavily used by the planner, and 
experiments are performed with a limit of 4GB or less, compiling in 32-bits is 
highly recommended.

But probably this is not a problem since FastDownward does not use this type of 
data-structure.
msg6671 (view) Author: jendrik Date: 2017-12-01.10:27:16
Building in 32-bit mode has several disadvantages:

- It only allows using up to 4 GiB of memory.
- It complicates our build scripts.
- It is not supported by recent CPLEX versions.

Therefore, we would like to remove or at least deprecate the option to build in 
32-bit mode. Currently, 64-bit builds don't match the performance of 32-bit 
builds, mainly because they use too much memory. Fixing this is issue213. We 
won't change the default build mode until issue213 is resolved. For now, we would 
like to collect possible concerns about deprecating or removing 32-bit builds. If 
somebody still needs 32-bit builds, we will probably only deprecate them and 
change the default to 64-bit. Otherwise, 32-bit builds will probably be removed.
History
Date User Action Args
2019-01-21 12:22:56maltesetmessages: + msg8485
2019-01-18 17:08:53floriansetstatus: in-progress -> resolved
messages: + msg8472
summary: Blocked by issue213. ->
2018-12-15 16:42:43maltesetmessages: + msg8359
title: make 64-bit builds the default, then remove option to build in 32-bit mode -> Remove options to build in 32-bit/64-bit mode; use machine/user defaults instead
2018-12-14 17:05:36floriansetmessages: + msg8353
2018-12-14 15:52:19maltesetmessages: + msg8349
2018-12-14 15:42:59floriansetmessages: + msg8347
2018-12-14 15:35:54maltesetmessages: + msg8346
2018-12-14 15:32:08floriansetmessages: + msg8345
2018-12-14 15:05:10maltesetmessages: + msg8342
2018-12-14 14:46:59floriansetmessages: + msg8341
2018-12-14 14:41:03jendriksetmessages: + msg8340
2018-12-14 13:56:15maltesetmessages: + msg8332
2018-12-14 13:52:04floriansetmessages: + msg8331
2018-12-14 12:16:22jendriksetmessages: + msg8326
2018-12-14 12:13:30silvansetmessages: + msg8325
2018-12-14 12:01:55maltesetmessages: + msg8324
2018-12-14 11:38:38floriansetassignedto: jendrik -> florian
messages: + msg8319
2018-09-13 12:23:08maltesetmessages: + msg7429
2018-09-13 12:16:32jendriksetstatus: chatting -> in-progress
messages: + msg7428
2018-09-13 12:07:03jendriksetassignedto: jendrik
messages: + msg7426
2018-09-13 11:52:36maltesetmessages: + msg7423
2018-09-11 15:43:21gabisetmessages: + msg7369
2018-09-10 17:44:50floriansetnosy: + gabi
messages: + msg7359
2017-12-01 12:56:11jendriksetmessages: + msg6680
2017-12-01 12:03:27maltesetmessages: + msg6676
2017-12-01 12:01:29maltesettitle: deprecate or remove option to build in 32-bit mode -> make 64-bit builds the default, then remove option to build in 32-bit mode
2017-12-01 12:00:56maltesetmessages: + msg6675
2017-12-01 11:46:14guillemsetnosy: + guillem
2017-12-01 11:13:54atorralbasetstatus: unread -> chatting
nosy: + atorralba
messages: + msg6673
2017-12-01 10:43:56floriansetnosy: + florian
2017-12-01 10:35:09silvansetnosy: + silvan
2017-12-01 10:27:16jendrikcreate