servo / webrender

A GPU-based renderer for the web
https://doc.servo.org/webrender/
Mozilla Public License 2.0
3.12k stars 277 forks source link

Improve get_scroll_node_state #3732

Open gterzian opened 5 years ago

gterzian commented 5 years ago

https://github.com/servo/webrender/blob/1e044fe1862373a055dbfb39930ead035e55b30f/webrender_api/src/api.rs#L1345

The above call is blocking, and although I'm not familiar with the overall communication patterns of the components involved, I can imagine it could beneficial to not block on the reply, instead finding a way to get it as part of the general event-loop where the code is running.

The additional problem is that the blocking reply is received by creating an ipc-channel, and sending the sender half on an ipc-channel, at each call, and that can eventually crash the environment where webrender is running, for example Servo.

A quick band-aid fix for half the problem would be re-using a channel, as opposed to creating a new one for each call(this only fixes half the problem because sending a clone of the sender still creates an fd for each clone when it is received in the other process).

See https://github.com/servo/servo/issues/23905#issuecomment-518785095

And the matching issue: https://github.com/servo/ipc-channel/issues/240

called `Result::unwrap()` on an `Err` value: Os { code: 24, kind: Other, message: "Too many open files" } (thread main, at src/libcore
/result.rs:1051)
failed to create IPC channel: Os { code: 24, kind: Other, message: "Too many open files" } (thread LayoutThread PipelineId { namespace
_id: PipelineNamespaceId(2), index: PipelineIndex(598) }, at src/libcore/result.rs:1051)
stack backtrace:
   0: servo::main::{{closure}}::he6dd7bec62c94245 (0x55acfaa4168f)
   1: std::panicking::rust_panic_with_hook::h0529069ab88f357a (0x55acfdc9c525)
             at src/libstd/panicking.rs:481
   2: std::panicking::continue_panic_fmt::h6a820a3cd2914e74 (0x55acfdc9bfc1)
             at src/libstd/panicking.rs:384
   3: rust_begin_unwind (0x55acfdc9bea5)
             at src/libstd/panicking.rs:311
   4: core::panicking::panic_fmt::he00cfaca5555542a (0x55acfdcbec0c)
             at src/libcore/panicking.rs:85
   5: core::result::unwrap_failed::h4239b9d80132a0db (0x55acfdcbed06)
             at src/libcore/result.rs:1051
   6: webrender_api::api::RenderApi::get_scroll_node_state::hd293254e30b6f279 (0x55acfdb5a837)
   7: compositing::compositor::IOCompositor<Window>::send_viewport_rects::had43b3860988bdba (0x55acfaa75900)
   8: _ZN11compositing10compositor26IOCompositor$LT$Window$GT$22handle_browser_message17h35ede344d68d17fbE.llvm.11108698028848252580 (
0x55acfaa76d50)
   9: compositing::compositor::IOCompositor<Window>::receive_messages::h9c21b79ecc7864f4 (0x55acfaa7453c)
  10: servo::Servo<Window>::handle_events::h2e8a235109d8b416 (0x55acfaa921b6)
  11: servo::app::App::handle_events::h719e551468bebb07 (0x55acfaa884c8)
  12: servo::app::App::run::hfb05858ec4b0a2e1 (0x55acfaa87278)
  13: servo::main::hbf1ae120024a1f3f (0x55acfaa40f97)
  14: std::rt::lang_start::{{closure}}::ha3e485d15a4449ac (0x55acfaa490a2)
  15: std::rt::lang_start_internal::{{closure}}::hbf11e2eac4637ca8 (0x55acfdc9be42)
             at src/libstd/rt.rs:49
      std::panicking::try::do_call::hce2b88a55d321203
             at src/libstd/panicking.rs:296
  16: __rust_maybe_catch_panic (0x55acfdca61d9)
             at src/libpanic_unwind/lib.rs:82
  17: std::panicking::try::hcfbcbb3944be5b74 (0x55acfdc9ca0c)
             at src/libstd/panicking.rs:275
      std::panic::catch_unwind::h65d3049f65e755e2
             at src/libstd/panic.rs:394
      std::rt::lang_start_internal::h97d8c129acb51f99
             at src/libstd/rt.rs:48
  18: main (0x55acfaa41841)
  19: __libc_start_main (0x7fd8c09bc82f)
  20: _start (0x55acfa9af6c8)
  21: <unknown> (0x0)
[2019-08-06T18:06:08Z ERROR servo] called `Result::unwrap()` on an `Err` value: Os { code: 24, kind: Other, message: "Too many open fi
les" }
stack b
gterzian commented 5 years ago

Here is a potential solution to make the operation non-blocking:

Could

https://github.com/servo/webrender/blob/1e5bf93a1efc799c48071c04165b2de1e972bd3e/webrender_api/src/api.rs#L1003

Take in a optional sender as argument(ipc-sender?), to be provided by the user of the API, on which various messages could be sent to the user of the API?

That way, something like get_scroll_node_state could just send the request to get the scroll-states, without blocking on the reply, and the reply would be communicated via the "sender" provided at the create_api call. It would then be the responseability of the API user to handle these messages and do what it needs to do with the scroll-states.

So get_scroll_node_state would become a way to "ask for an update", which would later come-in via one or several messages, to be handled by whatever is the appropriate event-loop that the user of the API is running?

it would also fix the issue of having to create an ipc-channel on each call, since the messaging would go over a previously established communication channel.

if no sender was provided in create_api, you could fallback to blocking mode.

kvark commented 5 years ago

It doesn't look like Gecko uses get_scroll_node_state anywhere, so we can shape it in the way Servo needs without worrying about breaking anything.