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

Split up the script crate for build speed #1799

Open
kmcallister opened this issue Mar 3, 2014 · 28 comments · May be fixed by #21371
Open

Split up the script crate for build speed #1799

kmcallister opened this issue Mar 3, 2014 · 28 comments · May be fixed by #21371
Labels
A-build Related to or part of the build process A-content/dom Interacting with the DOM from web content I-perf-slow Unnecessary performance degredation.

Comments

@kmcallister
Copy link
Contributor

It's by far the largest crate in terms of binary size (2.5× as big as main). Building it takes almost 40 seconds and 1.8 GB resident on my machine (with debugging enabled, optimization disabled).

The various DOM types and their generated bindings mostly don't depend on each other. We could split them among a few crates to improve compile time (especially in parallel).

@jdm
Copy link
Member

jdm commented Mar 3, 2014

See also #925. I'm not actually convinced that they're as detached as everyone thinks; each DOM constructor calls a reflect method which takes a bindings-generated function argument, along with using generated types for things like unions/dictionaries, while all of the bindings code interacts with non-generated DOM types anytime they're used as arguments.

@jdm
Copy link
Member

jdm commented Mar 3, 2014

That is to say, I don't see a way to split it so that one crate depends on the other; there are all sorts of bidirectional dependencies that I can't see a clear way to break.

@Ms2ger Ms2ger removed the E-easy label Mar 12, 2014
@mbrubeck mbrubeck added the A-build Related to or part of the build process label May 25, 2016
@jdm
Copy link
Member

jdm commented Oct 4, 2016

Splitting the script crate in a linear fashion may be more feasible. If we were able to move all of the generated bindings into a crate that script depends upon, that would mean that bindings and layout could build in parallel, and would reduce the working memory required to compile the rest of script. It would also speed up making changes to non-codegen parts of script.

@antrik
Copy link
Contributor

antrik commented Oct 4, 2016

I'd like to point out that this issue is more serious nowadays, as not only does the script crate totally dominate partial rebuild time (it takes several minutes for a non-optimised build now), but also the memory usage has gotten so large that it actually makes debug builds exceed the 3 GiB virtual memory available on 32 bit systems, as reported in #13045 -- and it's probably just a matter of time until non-debug builds break as well...

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 5, 2016

I don't see how that would work. The generated bindings need to be able to inline the method definitions on the concrete type, and the concrete type depends fundamentally on the wrap function, and the class / JSNative definitions with it.

@jdm
Copy link
Member

jdm commented Dec 9, 2016

Latest idea from discussions with @asajeffrey:

  • split generated bindings code into two crates - code that can be parameterized over FooTraits types (to avoid needing to import concrete DOM types), and code that needs to refer to the parameterized code using concrete types
  • extract low-level types from script/ that the generated bindings code needs to refer to (e.g. converting native types to JS values) and put them in the generic bindings crate

This should give us the following crates:
js_bindings (generic codegen, generated types (eg. unions, dictioanries), common script/ types)
script (DOM API implementations and non-generic codegen)

This would mean that non-WebIDL/CodegenRust.py changes should not have to rebuild a significant portion of the generated bindings code.

@jdm
Copy link
Member

jdm commented Dec 9, 2016

Things that will make the above idea difficult:

  • concrete DOM types in generated types (eg. unions or dictionaries containing WebIDL interface types)

@jdm
Copy link
Member

jdm commented Dec 9, 2016

At minimum, this will require collecting the list of WebIDL interfaces transitively used by a given WebIDL property (for each return value and argument), adding generic parameters for each, and propagating these generic types through layers like the union conversion methods when interacting with them.

@jdm
Copy link
Member

jdm commented Dec 10, 2016

Potential solution to the previous comment: a single trait with associated types for every WebIDL interface, and every generic method gets parameterized over the trait and uses T::Interface rather than Interface.

@asajeffrey
Copy link
Member

asajeffrey commented Dec 10, 2016

Or even better (?) make each FooMethods trait parametric, rather than each method inside FooMethods. Something like:

trait DOMTypes {
  type Foo: FooMethods<Self>;
  ...
}
trait FooMethods<DOM: DOMTypes> {
  fn bar(x: DOM::Bar) -> DOM::Baz;
  ...
}
struct DOM;
impl DOMTypes for DOM {
  type Foo = Foo;
  ...
}

struct Foo { ... }
impl FooMethods<DOM> for Foo { ... }

@jdm
Copy link
Member

jdm commented Dec 10, 2016

FooMethods would be unchanged. It's the standalone binding glue functions that need to be parametric.

@asajeffrey
Copy link
Member

How can FooMethods remain unchanged? Is the plan that FooMethods would be generated in the same crate as the concrete implementations? That does mean that the DOMTypes trait couldn't mention the FooMethods trait, but I don't know if that matters.

@jdm
Copy link
Member

jdm commented Dec 12, 2016

Oh right, FooMethods has arguments and return types too. Hooray!

@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 13, 2016

I wonder if we can use -> impl BarTypes now.

@asajeffrey
Copy link
Member

We have quite a lot of places in the script crate where we assume the WebIDL method returns the concrete type, e.g. that document.Window() returns a Root<Window> not an impl WindowMethods.

@jdm
Copy link
Member

jdm commented Dec 20, 2016

My recommendation for how to go about this - to sort out fundamental problems quickly, I propose copying the codegen infrastructure into a new project, taking some representative WebIDL files and DOM implementations, and doing whatever it necessary to make it build unmodified. At that point the rebuild time should be way smaller than Servo, which will yield much quicker feedback on experiments.

@asajeffrey
Copy link
Member

asajeffrey commented Mar 8, 2017

IRC conversation with @nox (initiated by @wafflespeanut): http://logs.glob.uno/?c=mozilla%23servo&s=8+Mar+2017&e=8+Mar+2017#c626950

TL;DR:

a) splitting script is speculative, it might or might not work,
b) it might all be rendered moot by rustc improving incremental computation,
c) it would be a truck load of work, and
d) we love having this debate.

@antrik
Copy link
Contributor

antrik commented Mar 8, 2017

@asajeffrey b) is not entirely true: while incremental compilation might help with the major pain point of long build times after small changes, initial build time is also somewhat relevant; and even more importantly, it won't help with the memory usage problem.

(Maybe other compiler improvements could help with the latter, though?...)

@asajeffrey
Copy link
Member

True, I was mainly thinking about speeding up recompiling when editing DOM .rs and .webidl files. You can tell which case hurts me!

@nox
Copy link
Contributor

nox commented Sep 29, 2017

I don't think this is useful to keep this issue open. We know that splitting the script crate is something we can consider, but this will be done so far in the future (if ever) that I am hoping by then incremental compilation will make the crate size a non-issue.

@nox nox closed this as completed Sep 29, 2017
@antrik
Copy link
Contributor

antrik commented Sep 29, 2017

@nox I want to point out, again, that incremental compilation will not make the memory footprint during build a non-issue.

@asajeffrey
Copy link
Member

The problem is still a real one, even if we don't have a clear vision of how to solve it, so I'd like to keep this issue open. If incremental computation makes this a non-issue, we can close the issue then.

@asajeffrey asajeffrey reopened this Sep 29, 2017
@SimonSapin
Copy link
Member

Building it takes almost 40 seconds

Hahaha, the good old times

@metux
Copy link

metux commented Dec 20, 2017

Any news on this issue ? Just having crashes by >4G memory consumption.

@jdm
Copy link
Member

jdm commented Dec 20, 2017

No news.

@metux
Copy link

metux commented Dec 20, 2017

hmm, I'll try with kicking out stuff I dont wanna have anyways (bluetooth, gamepad, vr, ...).
unfortunately, #[cfg(...)] doesn't seem to work in many places - I really miss a real preprocessor.

@haxxorsid
Copy link

Is the effort needed small, medium or xlarge?

@jdm
Copy link
Member

jdm commented Feb 10, 2018

The effort is still very large. I have proposed a Google Summer of Code project to investigate the ideas we have discussed in this thread.

hrvolapeter added a commit to hrvolapeter/servo-1 that referenced this issue Aug 9, 2018
Separation is based on adding types to TypeHolderTrait which contains
associated types with traits that script crate actually needs. This allows
using static methods and Sized types.

Affects servo in two ways:
- generic structs does not compile in script crate, but are left for later monomorphization
- dom_structs itself are moved to separate crate

TODO:
- [] tests
- [] performance
- [] polyshing

Fixes servo#1799

r? @jdm
hrvolapeter added a commit to hrvolapeter/servo-1 that referenced this issue Aug 9, 2018
Separation is based on adding types to TypeHolderTrait which contains
associated types with traits that script crate actually needs. This allows
using static methods and Sized types.

Affects servo in two ways:
- generic structs does not compile in script crate, but are left for later monomorphization
- dom_structs itself are moved to separate crate

TODO:
- [] tests
- [] performance
- [] polyshing

Fixes servo#1799

r? @jdm
@hrvolapeter hrvolapeter linked a pull request Aug 9, 2018 that will close this issue
8 tasks
bors-servo pushed a commit that referenced this issue Aug 9, 2018
[WIP] Generic script crate

Separation is based on adding types to TypeHolderTrait which contains
associated types with traits that script crate actually needs. This allows
using static methods and Sized types.

Affects servo in two ways:
- generic structs do not compile in script crate but are left for later monomorphization
- dom_structs itself are moved to separate crate

TODO:
- [ ] tests
- [ ] performance
- [ ] polyshing

Fixes #1799

r? @jdm

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #1799 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21371)
<!-- Reviewable:end -->
hrvolapeter added a commit to hrvolapeter/servo-1 that referenced this issue Aug 18, 2018
Separation is based on adding types to TypeHolderTrait which contains
associated types with traits that script crate actually needs. This allows
using static methods and Sized types.

Affects servo in two ways:
- generic structs does not compile in script crate, but are left for later monomorphization
- dom_structs itself are moved to separate crate

TODO:
- [] tests
- [] performance
- [] polyshing

Fixes servo#1799

r? @jdm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build Related to or part of the build process A-content/dom Interacting with the DOM from web content I-perf-slow Unnecessary performance degredation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants