#80 Stop using unified compilation of sources.

Open
opened 3 years ago by wolfbeast · 13 comments
wolfbeast commented 3 years ago (Migrated from github.com)

Mozilla has, for a while, been using unified source compilation, which concatenates multiple source files together before feeding the result to the compiler.
The idea behind this is that is creates shorter build times and theoretically allows the compiler to optimize more parts of the code.

There are, however, some major drawbacks to this that are considered detrimental to our work here:

  • Dependencies can be missed; if the concatenation includes another source file linking in the dependency, (e.g. a header), this header inclusion can be missing without the compiler complaining about it. If code changes and as a result the concatenation changes, we can suddenly be presented with build failures without a clear cause in sources that previously compiled without error.
  • It can create code bias for sources that happen to end up in the same block; this in turn can lead to timing differences, races, and performance gaps.
  • Debugging is more difficult; the actual compiled source file is a transitional chunk, not the actual source file.
  • The build is more fragile: it relies on extra steps performed, is sensitive to newlines at ends of files or not, etc.
  • Some source files can’t be built unified, others can, and whether one or the other is true can be difficult to discern.

There is also no conclusive data that the goal set, on our size of source tree, was actually achieved or not.

Mozilla has, for a while, been using unified source compilation, which concatenates multiple source files together before feeding the result to the compiler. The idea behind this is that is creates shorter build times and _theoretically_ allows the compiler to optimize more parts of the code. There are, however, some **major** drawbacks to this that are considered detrimental to our work here: - **Dependencies** can be missed; if the concatenation includes another source file linking in the dependency, (e.g. a header), this header inclusion can be missing without the compiler complaining about it. If code changes and as a result the concatenation changes, we can suddenly be presented with build failures without a clear cause in sources that previously compiled without error. - It can create **code bias** for sources that happen to end up in the same block; this in turn can lead to timing differences, races, and performance gaps. - **Debugging** is more **difficult**; the actual compiled source file is a transitional chunk, not the actual source file. - The build is more **fragile**: it relies on extra steps performed, is sensitive to newlines at ends of files or not, etc. - Some source files can't be built unified, others can, and whether one or the other is true can be difficult to discern. There is also **no conclusive data** that the goal set, on our size of source tree, was actually achieved or not.
wolfbeast commented 2 years ago (Migrated from github.com)

Some things to watch out for when working on this:

  • If classes/structs seem to be missing, check for using namespace statements as well as includes.
  • If running into undefined externals when linking, aside from getting the proper includes, be aware Mozilla employees have also sprinkled constexpr everywhere (because it’s new and cool...). If this is used on e.g. a forward declared constructor in a .h, this can lead to undefined external symbols at link time in non-unified mode if the relevant implementation is listed as constexpr -- it works when the caller and declaration containing .cpp files are concat-ed, but not when forward declared in the .h and implemented in the .cpp, because it will be evaluated and resolved at compile-time with constexpr. This little tidbit cost me 1.5 hours of scratching my head why the concated version worked but the separated version did not, while everything was included that was needed.
Some things to watch out for when working on this: - If classes/structs seem to be missing, check for `using namespace` statements as well as includes. - If running into undefined externals when linking, aside from getting the proper includes, be aware Mozilla employees have also sprinkled `constexpr` everywhere (because it's new and cool...). If this is used on e.g. a forward declared constructor in a `.h`, this can lead to undefined external symbols at link time in non-unified mode if the relevant implementation is listed as `constexpr` -- it works when the caller and declaration containing `.cpp` files are concat-ed, but not when forward declared in the `.h` and implemented in the `.cpp`, because it will be evaluated and resolved at **compile-time** with `constexpr`. This little tidbit cost me 1.5 hours of scratching my head why the concated version worked but the separated version did not, while everything was included that was needed.
wolfbeast commented 2 years ago (Migrated from github.com)

@adeshkp @g4jc @trav90
Proposed workflow for future deprot fixing to prevent build bustage on master:

deprot

@adeshkp @g4jc @trav90 Proposed workflow for future deprot fixing to prevent build bustage on master: ![deprot](https://user-images.githubusercontent.com/3359132/57178661-57cfab00-6e74-11e9-956e-632b002b81cd.png)
mattatobin commented 2 years ago (Migrated from github.com)

Sounds fantasic and didn’t need half a billion dollars to come up with a stratagy like that!

Sounds fantasic and didn't need half a billion dollars to come up with a stratagy like that!
trav90 commented 2 years ago (Migrated from github.com)

Sounds like a great plan!

Sounds like a great plan!
wolfbeast commented 1 year ago (Migrated from github.com)

We really shouldn’t be doing this on master directly, because... well see above.
Created a deunify_dom to continue this work; when other parts are tackled a similar deunify_* branch should be used.

We really shouldn't be doing this on `master` directly, because... well see above. Created a `deunify_dom` to continue this work; when other parts are tackled a similar `deunify_*` branch should be used.
mattatobin commented 1 year ago (Migrated from github.com)

@adeshkp @dbsoft Can one of you please build preferably Basilisk or simply enabling webrtc and eme (even if the application doesn’t support it) to check building and linking of the deunify_dom branch on Macintosh.

@adeshkp @dbsoft Can one of you please build preferably Basilisk or simply enabling webrtc and eme (even if the application doesn't support it) to check building and linking of the `deunify_dom` branch on Macintosh.
adeshkp commented 1 year ago (Migrated from github.com)

@adeshkp @dbsoft Can one of you please build preferably Basilisk or simply enabling webrtc and eme (even if the application doesn’t support it) to check building and linking of the deunify_dom branch on Macintosh.

I already have a patch for Basilisk building on the master branch but did not publish it as I wanted this thing to settle down (too much code churn). I’ll check now on the mentioned branch to see if changes are required for building/linking on Mac.

> @adeshkp @dbsoft Can one of you please build preferably Basilisk or simply enabling webrtc and eme (even if the application doesn't support it) to check building and linking of the `deunify_dom` branch on Macintosh. I already have a patch for Basilisk building on the master branch but did not publish it as I wanted this thing to settle down (too much code churn). I'll check now on the mentioned branch to see if changes are required for building/linking on Mac.
mattatobin commented 1 year ago (Migrated from github.com)

Thank you @adeshkp I don’t need a copy just a yay or nay at this point.

Thank you @adeshkp I don't need a copy just a yay or nay at this point.
adeshkp commented 1 year ago (Migrated from github.com)

https://github.com/MoonchildProductions/UXP/pull/1523

I have created a pull request for deunify_dom branch instead of master. I hope that is okay. If not, you can directly pick the two commits I made to my branch.

I also built on Linux and didn’t find an issue there. Someone else can also verify if needed.

https://github.com/MoonchildProductions/UXP/pull/1523 I have created a pull request for `deunify_dom` branch instead of master. I hope that is okay. If not, you can directly pick the two commits I made to my branch. I also built on Linux and didn't find an issue there. Someone else can also verify if needed.
mattatobin commented 1 year ago (Migrated from github.com)

@adeshkp Thank you very much, both Moonchild and my self, for this and the fixes.

@adeshkp Thank you very much, both Moonchild and my self, for this and the fixes.
wolfbeast commented 1 year ago (Migrated from github.com)

Marker comment -- /dom completed.

Marker comment -- `/dom` completed.
mattatobin commented 1 year ago (Migrated from github.com)

Status: /layout completed -- Unblocking #1375 from continuing

/gfx is next with /gfx/thebes being top of the list because gcc-10 seems to shift unified sources enough to make it fail.

Status: `/layout` completed -- Unblocking #1375 from continuing `/gfx` is next with `/gfx/thebes` being top of the list because `gcc-10` seems to shift unified sources enough to make it fail.
wolfbeast commented 1 year ago (Migrated from github.com)

@mattatobin gfx/thebes has been done. Not sure about buildability on Linux in general since it got rather thick with platform specific includes and what not, but you may be able to check if gcc 10 is happier with this?

@mattatobin gfx/thebes has been done. Not sure about buildability on Linux in general since it got rather thick with platform specific includes and what not, but you may be able to check if gcc 10 is happier with this?
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
Moonchild referenced this issue from a commit 1 year ago
This repo is archived. You cannot comment on issues.
No Milestone
No Assignees
1 Participants
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.