Skip to content

Meeting 2015 07 27

Jump to bottom
Lars Bergstrom edited this page Jul 27, 2015 · 1 revision

Agenda items

  • Time to turn off Critic & its webhook for real? (larsberg)
  • Tree closure for e10s (jack)
  • CI / e10s issues (jack)
    • release / debug mode runs
    • bincode status
    • intermittents
  • Hyper upgrade/OpenSSL issues (pcwalton)
  • e10s defaults (jack)

Last week

Attending

  • jack, mbrubeck, jdm, larsberg, brson, cimes, laleh, ms2ger, frewsxcv, manish, simonsapin, dherman

Turning off critic

  • larsberg: I saw it confuse a new contributor. I think we just wanted to wait until things were done. Can we at least turn off the webhook?
  • jack: Yeah, off for new reviews.
  • larsberg: Will do.

Tree closure for e10s

  • jack: pcwalton sent a message to the list.
  • pcwalton: Since I sent it, about half of the areas listed e10s has landed. Thanks to all who helped with it! Areas unblocked are canvas, webgl, storage task, devtools (pending current PR landing), and image cache task. Remaining: resource task, devtools (if it bounces), and displaylist. Hopefully displaylist in soon.
  • jack: I think everybody was fine, but let's get it landed ASAP. Can you send an update to dev-servo?
  • pcwalton: Sure. I suspect the worst is over, because resource task is sorta generic and not being touched much now.
  • jack: Only simon's rustup appears blocked...
  • jdm: There are a couple of others, too.

Hyper

  • pcwalton: Hyper upgrade is blocked because there was an OpenSSL change, and it picked something up that broke gonk. I know mbrubeck was looking into this. Plan?
  • mbrubeck: https://github.com/servo/servo/pull/6740#issuecomment-125273818. Problem is that our builders don't have the OpenSSL headers in the gonk image.
  • larsberg: I can update those.
  • pcwalton: That should be it.

Intermittents

  • pcwalton: Canvas issue. Probably will go away once we have bincode. It's probably the result of serialization being too slow and causing a timeout. We had a lot of test issues when IPC channel was leaking file descriptors, but that's fixed.
  • jdm: I filed two separate canvas timeouts. There are also at least two other timeouts with odd log messages. Maybe don't point at the IPC stuff, but they all happened since then? The Canvas ones are very frequent and blocking many things right now. Frustrating either disabling them or leaving the intermittent.
  • pcwalton: Maybe LONG timeout for them?
  • jdm: Can try it.
  • jack: Let's try that. Otherwise, does WPT have a way to say they're intermittent to not gate on them? Like we have on reftests?
  • jdm: That's just disabling them, effectively.
  • jack: What's the story with bincode?
  • pcwalton: Not sure. Erickt had something ready, but he's very busy right now. He's the only maintainer on serde, unfortunately. He's splitting it into two parts: first API changes for bincode and then merging it. He has an RFC for users about the API changes, which has positive feedback. Long-term, unless his schedule frees up, one of us will have to help maintain it.
  • jack: Worried about our perf numbers for reporting right now. If we're landing with big perf regressions, this is not optimal.
  • pcwalton: I'll get in touch with erickt as soon as I can. If nothing else, there is a fork of serde with bincode that we could just fork to or git-depend on.
  • jack: Change every dependency?
  • pcwalton: Yes. Not ideal. Just depends on the type of benchmark you're doing. Mainly displaylist serialization, which we will not use anyway for the single-process version. I have a tentative long-term plan, but not doing it for now. Mainly see issues when we have a lot if issues or are benchmarking network/localstorage/canvas/webgl performance. Also not entirely clear that it'll be enough for webgl+canvas and may have to buffer.

e10s default

  • jack: Should it be on by default?
  • pcwalton: No. The displaylist perf issue is too bad to turn it on by default now. I have a plan, but before I can even do that we need bincode support. Until then, no multiprocess by default. We will want single process in perpetuity, even if only for debugging.
  • jack: Need it anyway; some of our partners want single-process deployments of Servo.
  • pcwalton: Shouldn't be a challenge. The spawn code is ~10 lines in constellation.rs, so there isn't a huge fork. Equally important is the no sandbox mode, since you can't debug if it's sandboxed. Should start to run tests on multiprocess / tests, though. Reviewers should be watching for forbidden syscalls in the content process.
  • jdm: Please write that down and send it to the mailing list.
  • jack: Is there a guide in the e10s patch?
  • pcwalton: Doc is scattered. script+layout are in one process; everything else in the other, with IPC channels in between. There's doc in the readme for the IPC stuff. No rustdoc comments yet, though. The API is still pretty high-churn while we figure out if everything is working for Servo. There is a design doc / readme, though. There's also a readme in gaol, which is the sandboxing stuff, that explains how all that works on OSX and Linux. This is a cross-platform sandboxing library.
  • jack: At least need an unscattered version of what the architecture is.
  • pcwalton: I'll send something to the list and put it on the wiki.
  • jack: Yes.

release vs. debug

  • larsberg: There were failures when I switched us from dev to release WPT builders. I want to spin up and test both. Do we understand what's going on with the dev-only & release-only failures and especially the differences.
  • jdm: Not totally understood yet.
  • pcwalton: I profiled on linux. We have a lot of timeouts. There's a bunch of code for reassembling network fragmented packet reassembly, and in debug the Rust code is impossibly slow. Ultimately, I'd like to see Cargo be able to build an optimized IPC channel implementation even in a dev/debug build. IPC channel does have a unit test suite to cover the lack of coverage of debug IPC channels.
  • brson: IPC channels are generic, right?
  • pcwalton: Yes.
  • brson: Selective optimization is going to be way more complex than that...
  • pcwalton: The guts are not generic, though. It's just over bytebuffer<u8>. Good point, though!
  • jack: Would also be weird if we relied on cross-crate inlining, but that's not the case here.
  • pcwalton: I submitted a patch, but it was r-'d by the Core Cargo Team and needs more work.
  • simonsapin: To work around this kind of limitation, we can run cargo with the RUSTC variable set to a script. We already do this for rustdoc to build docs with private items.
  • jack: Seems reasonable to compile it separately and see if that fixes WPT. If so, we can put the workaround in and work with the Cargo team to figure out how to solve this.
  • pcwalton: I think I know how they want it solved - there's a sketch in the PR. But it's not high-pri. Popping the stack, there's another issue where the class inheritance hierarchy of JS objects seems to depend on the optimization level that script was compiled with. Which is concerning.
  • jdm: There's a test where the result changed from pass to fail that just checks the prototype of the image element and constructor. I'm going to look into that.
  • jack: That's terrifying.
  • pcwalton: Yes.
  • jack: Do we turn something off in SM or the bindings when we compile with optimization?
  • jdm: Hard to answer.
  • manish: If it's in inheritance, that whole model uses UB. We assume struct layout is constant, but if Rust does any optimizations, it would break that...
  • pcwalton: I don't think so.
  • jack: Can't we just repr(C) to fix that?
  • manish: Yeah, we could do that.

e10s wrapup

  • pcwalton: Thanks everyone for your patience getting e10s landed.
  • larsberg: Do we have any lints or static checks to ensure we don't do non-e10s-friendly things in the future?
  • pcwalton: There are two ways things can go wrong. You could add new non-serializable things to existing messages. derive(Serialize) will catch that. The other way it can wrong is adding entirely new channels between content and chrome that aren't IPC channels. Once we are running with IPC enabled, that will cause failures too. So the type system should be preventing any radically non-e10s-safe things from landing. I would like to run tests in e10s mode at some point, but just having it built will go a long way.
  • jack: Can you file a bug to add e10s tests, just so we don't forget it?
  • pcwalton: Sure.
  • jack: If we don't prioritize the work to make this the default mode, it's likely to stay slow and be a second-class-citizen.
  • pcwalton: From a big picture view, the most challenging part will be making the display list fast. Other browsers have proven that overall e10s is not that big an issue, but display lists are the tough part. I think we can do it, but SkPicture serialization time is essentially an unsolved problem, not just for us but for Chromium too.
Clone this wiki locally