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

WebIDL codegen: Replace cmake with a single Python script #24303

Merged
merged 2 commits into from Sep 30, 2019
Merged

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Sep 27, 2019

When playing around with Cargo’s new timing visualization, I was surprised to see the script crate’s build script take 76 seconds. I did not expect WebIDL bindings generation to be that computationally intensive.

It turns out almost all of this time is overhead. The build script uses CMake to generate bindings for each WebIDL file in parallel, but that causes a lot of work to be repeated 366 times:

  • Starting up a Python VM
  • Importing (parts of) the Python standard library
  • Importing ~16k lines of our Python code
  • Recompiling the latter to bytecode, since we used python -B to disable writing .pyc files
  • Deserializing with cPickle and recreating in memory the results of parsing all WebIDL files

This commit remove the use of CMake and cPickle for the script crate. Instead, all WebIDL bindings generation is done sequentially in a single Python process. This takes 2 to 3 seconds.


This change is Reviewable

@SimonSapin SimonSapin force-pushed the script-codegen branch from 3b1305f to 7b92fa4 Sep 27, 2019
@CYBAI
Copy link
Collaborator

CYBAI commented Sep 27, 2019

Tidy and fmt complain 👀


Checking files for tidiness...

�[94m./components/script/dom/bindings/codegen/run.py�[0m:�[93m9�[0m: �[91mE302 expected 2 blank lines, found 1�[0m

�[94m./components/script/dom/bindings/codegen/run.py�[0m:�[93m31�[0m: �[91mF841 local variable 'bindings_dir' is assigned to but never used�[0m

�[94m./components/script/dom/bindings/codegen/run.py�[0m:�[93m70�[0m: �[91mE302 expected 2 blank lines, found 1�[0m

Running the dependency licensing lint...
�[1minfo: �(B�[minstalling component 'rustfmt'
Diff in /repo/components/script/build.rs at line 30:
         std::process::exit(1)
     }
 
�[31m-    println!(
�(B�[m�[31m-        "Binding generation completed in {:?}",
�(B�[m�[31m-        start.elapsed()
�(B�[m�[31m-    );
�(B�[m�[32m+    println!("Binding generation completed in {:?}", start.elapsed());
�(B�[m 
     let json = out_dir.join("InterfaceObjectMapData.json");
     let json: Value = serde_json::from_reader(File::open(&json).unwrap()).unwrap();
Run `./mach fmt` to fix the formatting
@SimonSapin SimonSapin force-pushed the script-codegen branch from 7b92fa4 to 098a528 Sep 27, 2019
@nox
Copy link
Member

nox commented Sep 27, 2019

Really great, @SimonSapin!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2019

📌 Commit 098a528 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2019

Testing commit 098a528 with merge ffde638...

bors-servo added a commit that referenced this pull request Sep 27, 2019
WebIDL codegen: Replace cmake with a single Python script

When [playing around with Cargo’s new timing visualization](https://internals.rust-lang.org/t/exploring-crate-graph-build-times-with-cargo-build-ztimings/10975/21), I was surprised to see the `script` crate’s build script take 76 seconds. I did not expect WebIDL bindings generation to be *that* computationally intensive.

It turns out almost all of this time is overhead. The build script uses CMake to generate bindings for each WebIDL file in parallel, but that causes a lot of work to be repeated 366 times:

* Starting up a Python VM
* Importing (parts of) the Python standard library
* Importing ~16k lines of our Python code
* Recompiling the latter to bytecode, since we used `python -B` to disable writing `.pyc` files
* Deserializing with `cPickle` and recreating in memory the results   of parsing all WebIDL files

----

This commit remove the use of CMake and cPickle for the `script` crate. Instead, all WebIDL bindings generation is done sequentially in a single Python process. This takes 2 to 3 seconds.

<!-- 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/24303)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2019

💔 Test failed - linux-rel-css

@nox
Copy link
Member

nox commented Sep 27, 2019

@bors-servo retry

 ▶ CRASH [expected OK] /referrer-policy/generic/iframe-src-change.html
  │ 
  │ 
  │ 
  │ 
  │ 
  │ (servo:53352): GStreamer-WARNING **: External plugin loader failed. This most likely means that the plugin loader helper binary was not found or could not be run. You might need to set the GST_PLUGIN_SCANNER environment variable if your setup is unusual. This should normally not be required though.
  │ 0:00:00.019360823 53352 0x557b35606930 WARN      GST_PLUGIN_LOADING gstplugin.c:793:_priv_gst_plugin_load_file_for_registry: module_open failed: libmms.so.0: cannot open shared object file: No such file or directory
  │ 
  │ (servo:53352): GStreamer-WARNING **: Failed to load plugin '/home/servo/gst/lib/gstreamer-1.0/libgstmms.so': libmms.so.0: cannot open shared object file: No such file or directory
  │ 0:00:00.019593337 53352 0x557b35606930 WARN      GST_PLUGIN_LOADING gstplugin.c:793:_priv_gst_plugin_load_file_for_registry: module_open failed: libfaad.so.2: cannot open shared object file: No such file or directory
  │ 
  │ (servo:53352): GStreamer-WARNING **: Failed to load plugin '/home/servo/gst/lib/gstreamer-1.0/libgstfaad.so': libfaad.so.2: cannot open shared object file: No such file or directory
  │ Layout thread disconnected.: "SendError(..)" (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(1) }, at src/libcore/result.rs:1165)
  │ stack backtrace:
  │    0: servo::main::{{closure}}::h65e3e684a2581c90 (0x557b2f5af5ad)
  │    1: std::panicking::rust_panic_with_hook::h12d7650e86d2fcb0 (0x557b3285c1bc)
  │              at src/libstd/panicking.rs:477
  │    2: std::panicking::continue_panic_fmt::h70eeb0d1820fa233 (0x557b3285bc72)
  │              at src/libstd/panicking.rs:380
  │    3: rust_begin_unwind (0x557b3285bb66)
  │              at src/libstd/panicking.rs:307
  │    4: core::panicking::panic_fmt::hbbf14b8c86c6fc9d (0x557b3287e66a)
  │              at src/libcore/panicking.rs:85
  │    5: core::result::unwrap_failed::hc083e639e8c43c50 (0x557b3287e767)
  │              at src/libcore/result.rs:1165
  │    6: script::dom::window::Window::force_reflow::he7247588e4d0c578 (0x557b30004f3e)
  │    7: script::dom::window::Window::reflow::h4256787a06a975d9 (0x557b300056c7)
  │    8: script::dom::document::Document::finish_load::h929443ee122b843d (0x557b3026aaac)
  │    9: script::dom::servoparser::ServoParser::do_parse_sync::h0ac7393f1b428b9b (0x557b30118270)
  │   10: profile_traits::time::profile::h700f46dc199c3c2a (0x557b30373117)
  │   11: _ZN6script3dom11servoparser11ServoParser10parse_sync17h8e0da825f09547aeE.llvm.11410442732202748299 (0x557b301178e9)
  │   12: <script::dom::servoparser::ParserContext as net_traits::FetchResponseListener>::process_response_eof::hbc72f32d7d1f5c31 (0x557b3011d91a)
  │   13: script::script_thread::ScriptThread::handle_msg_from_constellation::h6fdf4165eaeec76b (0x557b302e8786)
  │   14: _ZN6script13script_thread12ScriptThread11handle_msgs17h58bdf7ce89706c46E.llvm.17017111312681008366 (0x557b302e085a)
  │   15: script::script_thread::ScriptThread::start::hc31ad0df28e79fa1 (0x557b302dab28)
  │   16: profile_traits::mem::ProfilerChan::run_with_memory_reporting::h8688d2e7b5f7229b (0x557b3044159d)
  │   17: std::sys_common::backtrace::__rust_begin_short_backtrace::h03b2e4457b48a654 (0x557b3075b6f9)
  │   18: _ZN3std9panicking3try7do_call17hf1b51c7cdc500afcE.llvm.7711134062402776750 (0x557b308708a4)
  │   19: __rust_maybe_catch_panic (0x557b32865c9a)
  │              at src/libpanic_unwind/lib.rs:80
  │   20: core::ops::function::FnOnce::call_once{{vtable.shim}}::h4a4ff405e3f620a9 (0x557b2ff6ed13)
  │   21: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::h1d0ecaf281abdf71 (0x557b3284aa2f)
  │              at /rustc/66bf391c3aabfc77f5f7139fc9e6944f995d574e/src/liballoc/boxed.rs:922
  │   22: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::hfc866af69ec8f7e1 (0x557b32864fe0)
  │              at /rustc/66bf391c3aabfc77f5f7139fc9e6944f995d574e/src/liballoc/boxed.rs:922
  │       std::sys_common::thread::start_thread::h884843b1b0377783
  │              at src/libstd/sys_common/thread.rs:13
  │       std::sys::unix::thread::Thread::new::thread_start::hf370570edee1b7e3
  │              at src/libstd/sys/unix/thread.rs:79
  │   23: start_thread (0x7f6e4943a6ba)
  │   24: clone (0x7f6e47cd541d)
  │   25: <unknown> (0x0)
  │ [2019-09-27T09:34:29Z ERROR servo] Layout thread disconnected.: "SendError(..)"
  └ Pipeline failed in hard-fail mode.  Crashing!
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2019

Testing commit 098a528 with merge 0438c8a...

bors-servo added a commit that referenced this pull request Sep 27, 2019
WebIDL codegen: Replace cmake with a single Python script

When [playing around with Cargo’s new timing visualization](https://internals.rust-lang.org/t/exploring-crate-graph-build-times-with-cargo-build-ztimings/10975/21), I was surprised to see the `script` crate’s build script take 76 seconds. I did not expect WebIDL bindings generation to be *that* computationally intensive.

It turns out almost all of this time is overhead. The build script uses CMake to generate bindings for each WebIDL file in parallel, but that causes a lot of work to be repeated 366 times:

* Starting up a Python VM
* Importing (parts of) the Python standard library
* Importing ~16k lines of our Python code
* Recompiling the latter to bytecode, since we used `python -B` to disable writing `.pyc` files
* Deserializing with `cPickle` and recreating in memory the results   of parsing all WebIDL files

----

This commit remove the use of CMake and cPickle for the `script` crate. Instead, all WebIDL bindings generation is done sequentially in a single Python process. This takes 2 to 3 seconds.

<!-- 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/24303)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2019

💔 Test failed - status-taskcluster

When playing around with Cargo’s new timing visualization:
https://internals.rust-lang.org/t/exploring-crate-graph-build-times-with-cargo-build-ztimings/10975/21

… I was surprised to see the `script` crate’s build script take 76 seconds.
I did not expect WebIDL bindings generation to be *that* computationally
intensive.

It turns out almost all of this time is overhead. The build script uses CMake
to generate bindings for each WebIDL file in parallel, but that causes a lot
of work to be repeated 366 times:

* Starting up a Python VM
* Importing (parts of) the Python standard library
* Importing ~16k lines of our Python code
* Recompiling the latter to bytecode, since we used `python -B` to disable
  writing `.pyc` file
* Deserializing with `cPickle` and recreating in memory the results
  of parsing all WebIDL files

----

This commit remove the use of CMake and cPickle for the `script` crate.
Instead, all WebIDL bindings generation is done sequentially
in a single Python process. This takes 2 to 3 seconds.
@SimonSapin SimonSapin force-pushed the script-codegen branch from 098a528 to 5c60023 Sep 27, 2019
@SimonSapin
Copy link
Member Author

SimonSapin commented Sep 27, 2019

Fixed a Windows-specific failure:

error: couldn't read C:\Users\task_1569575870\repo\target\aarch64-pc-windows-msvc\debug\build\script-22dda419fb071ee6\out/build/InterfaceTypes.rs: The system cannot find the path specified. (os error 3)
   --> components\script\dom\mod.rs:212:5
    |
212 |     include!(concat!(env!("OUT_DIR"), "/build/InterfaceTypes.rs"));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

@bors-servo r=nox

@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2019

📌 Commit 5c60023 has been approved by nox

bors-servo added a commit that referenced this pull request Sep 27, 2019
WebIDL codegen: Replace cmake with a single Python script

When [playing around with Cargo’s new timing visualization](https://internals.rust-lang.org/t/exploring-crate-graph-build-times-with-cargo-build-ztimings/10975/21), I was surprised to see the `script` crate’s build script take 76 seconds. I did not expect WebIDL bindings generation to be *that* computationally intensive.

It turns out almost all of this time is overhead. The build script uses CMake to generate bindings for each WebIDL file in parallel, but that causes a lot of work to be repeated 366 times:

* Starting up a Python VM
* Importing (parts of) the Python standard library
* Importing ~16k lines of our Python code
* Recompiling the latter to bytecode, since we used `python -B` to disable writing `.pyc` files
* Deserializing with `cPickle` and recreating in memory the results   of parsing all WebIDL files

----

This commit remove the use of CMake and cPickle for the `script` crate. Instead, all WebIDL bindings generation is done sequentially in a single Python process. This takes 2 to 3 seconds.

<!-- 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/24303)
<!-- Reviewable:end -->
@nox
Copy link
Member

nox commented Sep 27, 2019

💡 This pull request was already approved, no need to approve it again.

Are you drunk, @bors-servo?

@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2019

💔 Test failed - linux-rel-css

@nox
Copy link
Member

nox commented Sep 29, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2019

Testing commit 5c60023 with merge 04f47ee...

bors-servo added a commit that referenced this pull request Sep 29, 2019
WebIDL codegen: Replace cmake with a single Python script

When [playing around with Cargo’s new timing visualization](https://internals.rust-lang.org/t/exploring-crate-graph-build-times-with-cargo-build-ztimings/10975/21), I was surprised to see the `script` crate’s build script take 76 seconds. I did not expect WebIDL bindings generation to be *that* computationally intensive.

It turns out almost all of this time is overhead. The build script uses CMake to generate bindings for each WebIDL file in parallel, but that causes a lot of work to be repeated 366 times:

* Starting up a Python VM
* Importing (parts of) the Python standard library
* Importing ~16k lines of our Python code
* Recompiling the latter to bytecode, since we used `python -B` to disable writing `.pyc` files
* Deserializing with `cPickle` and recreating in memory the results   of parsing all WebIDL files

----

This commit remove the use of CMake and cPickle for the `script` crate. Instead, all WebIDL bindings generation is done sequentially in a single Python process. This takes 2 to 3 seconds.

<!-- 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/24303)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2019

💔 Test failed - status-taskcluster

@CYBAI
Copy link
Collaborator

CYBAI commented Sep 29, 2019

Failure in Windows x64: Dev build + unit tests build 👀 Is it intermittent ? 🤔

error: failed to run custom build command for `script v0.0.1 (C:\Users\task_1569752260\repo\components\script)`

Caused by:
  process didn't exit successfully: `C:\Users\task_1569752260\repo\target\debug\build\script-a0e9dabea58adc15\build-script-build` (exit code: 101)
--- stderr
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src\libcore\option.rs:378:21
stack backtrace:
   0: std::sys_common::backtrace::_print::{{impl}}::fmt
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libstd\sys_common\backtrace.rs:60
   1: core::fmt::write
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libcore\fmt\mod.rs:1028
   2: std::io::Write::write_fmt<std::sys::windows::stdio::Stderr>
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libstd\io\mod.rs:1412
   3: std::panicking::default_hook::{{closure}}
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libstd\panicking.rs:196
   4: std::panicking::default_hook
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libstd\panicking.rs:210
   5: std::panicking::rust_panic_with_hook
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libstd\panicking.rs:473
   6: std::panicking::continue_panic_fmt
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libstd\panicking.rs:380
   7: std::panicking::rust_begin_panic
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libstd\panicking.rs:307
   8: core::panicking::panic_fmt
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libcore\panicking.rs:84
   9: core::panicking::panic
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libcore\panicking.rs:49
  10: core::option::Option<std::ffi::os_str::OsString>::unwrap<std::ffi::os_str::OsString>
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\src\libcore\macros.rs:12
  11: build_script_build::main
             at .\build.rs:20
  12: std::rt::lang_start::{{closure}}<()>
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\src\libstd\rt.rs:64
  13: std::panicking::try::do_call<closure-0,i32>
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libstd\panicking.rs:292
  14: panic_unwind::__rust_maybe_catch_panic
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libpanic_unwind\lib.rs:80
  15: std::rt::lang_start_internal
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\/src\libstd\rt.rs:48
  16: std::rt::lang_start<()>
             at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c\src\libstd\rt.rs:64
  17: main
  18: __scrt_common_main_seh
             at d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
  19: BaseThreadInitThunk
  20: RtlUserThreadStart
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
@nox
Copy link
Member

nox commented Sep 29, 2019

I'm 99.99% sure this happened before, I think @jdm solved it last time.

@SimonSapin
Copy link
Member Author

SimonSapin commented Sep 30, 2019

I’ve filed rust-lang/cargo#7458 to ask around.

@nox
Copy link
Member

nox commented Sep 30, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Sep 30, 2019

📌 Commit d2c299a has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Sep 30, 2019

Testing commit d2c299a with merge 402db83...

bors-servo added a commit that referenced this pull request Sep 30, 2019
WebIDL codegen: Replace cmake with a single Python script

When [playing around with Cargo’s new timing visualization](https://internals.rust-lang.org/t/exploring-crate-graph-build-times-with-cargo-build-ztimings/10975/21), I was surprised to see the `script` crate’s build script take 76 seconds. I did not expect WebIDL bindings generation to be *that* computationally intensive.

It turns out almost all of this time is overhead. The build script uses CMake to generate bindings for each WebIDL file in parallel, but that causes a lot of work to be repeated 366 times:

* Starting up a Python VM
* Importing (parts of) the Python standard library
* Importing ~16k lines of our Python code
* Recompiling the latter to bytecode, since we used `python -B` to disable writing `.pyc` files
* Deserializing with `cPickle` and recreating in memory the results   of parsing all WebIDL files

----

This commit remove the use of CMake and cPickle for the `script` crate. Instead, all WebIDL bindings generation is done sequentially in a single Python process. This takes 2 to 3 seconds.

<!-- 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/24303)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 30, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: nox
Pushing 402db83 to master...

@bors-servo bors-servo merged commit d2c299a into master Sep 30, 2019
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@SimonSapin SimonSapin deleted the script-codegen branch Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.