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

Improve get_scroll_node_state #3732

Open
gterzian opened this issue Aug 7, 2019 · 2 comments
Open

Improve get_scroll_node_state #3732

gterzian opened this issue Aug 7, 2019 · 2 comments

Comments

@gterzian
Copy link
Member

gterzian commented Aug 7, 2019

pub fn get_scroll_node_state(&self, document_id: DocumentId) -> Vec<ScrollNodeState> {

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 servo/servo#23905 (comment)

And the matching issue: servo/ipc-channel#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 gterzian changed the title Make get_scroll_node_state non-blocking Improve get_scroll_node_state Aug 7, 2019
@gterzian
Copy link
Member Author

gterzian commented Aug 7, 2019

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

Could

pub fn create_api(&self) -> RenderApi {

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
Copy link
Member

kvark commented Aug 8, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants