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

script: Make the `ImageCacheTask` use IPC. #6597

Merged
merged 1 commit into from Jul 27, 2015

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Jul 11, 2015

This necessitated getting rid of the boxed trait object that was being
be passed between the script task and the image cache task.

r? @jdm

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jul 11, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5523

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jdm
Copy link
Member

jdm commented Jul 14, 2015

-S-awaiting-review +S-needs-code-changes
The other Cargo.lock files will need updating too. Otherwise this basically fine; just a couple small bits that need addressing!


Reviewed 17 of 17 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


components/layout/layout_task.rs, line 317 [r1] (raw file):
nit: remove extra newline


components/script/dom/htmlimageelement.rs, line 89 [r1] (raw file):
I think this should be an unwrap until we figure out what to do in an error case.


components/script/dom/htmlimageelement.rs, line 91 [r1] (raw file):
Use a Runnable here instead.


components/script/dom/htmlimageelement.rs, line 98 [r1] (raw file):
This doesn't need to be public if we use a runnable above.


components/script/dom/htmlimageelement.rs, line 153 [r1] (raw file):
Add a TODO here to create an IPC thread that can be shared by all responders for a given script task.


components/script/script_task.rs, line 204 [r1] (raw file):
This can be removed if we use a runnable as suggested earlier.


Comments from the review on Reviewable.io

@pcwalton pcwalton force-pushed the pcwalton:image-cache-ipc branch from dd10f8c to 4635c81 Jul 14, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 14, 2015

r? @jdm

I believe everything is addressed now.

@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

The latest upstream changes (presumably #6596) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Jul 14, 2015

@bors-servo: r+
-S-awaiting-review +S-awaiting-merge


Reviewed 19 of 19 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@jdm jdm removed the S-awaiting-review label Jul 14, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

📌 Commit 4635c81 has been approved by jdm

@jdm jdm added S-needs-rebase and removed S-awaiting-merge labels Jul 14, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

🔒 Merge conflict

@pcwalton pcwalton force-pushed the pcwalton:image-cache-ipc branch from 4635c81 to 2780d54 Jul 14, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 14, 2015

Rebased. Now blocked on serde-rs/serde#102.

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2015

The latest upstream changes (presumably #6618) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm jdm added the S-needs-rebase label Jul 15, 2015
@pcwalton pcwalton force-pushed the pcwalton:image-cache-ipc branch from 2780d54 to 0818ba3 Jul 16, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 16, 2015

I have updated the patch to remove all additional helper threads (other the one per process that ipc-channel creates).

r? @jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 16, 2015

The latest upstream changes (presumably #6583) made this pull request unmergeable. Please resolve the merge conflicts.

@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 25, 2015

@bors-servo: r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 25, 2015

📌 Commit 53fe692 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 25, 2015

Testing commit 53fe692 with merge 1394fae...

bors-servo pushed a commit that referenced this pull request Jul 25, 2015
script: Make the `ImageCacheTask` use IPC.

This necessitated getting rid of the boxed trait object that was being
be passed between the script task and the image cache task.

r? @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6597)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 25, 2015

💔 Test failed - linux3

@jdm jdm added S-tests-failed and removed S-awaiting-merge labels Jul 25, 2015
@jdm
Copy link
Member

jdm commented Jul 25, 2015

/css21_dev/html4/abspos-replaced-width-margin-000.htm
-----------------------------------------------------
CRASH expected FAIL [Parent]

1:07.33 TEST_START: Thread-TestrunnerManager-6 /css21_dev/html4/abspos-replaced-width-margin-000.htm
 1:08.03 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) Full command: /home/servo/buildbot/slave/linux3/build/target/release/servo --cpu --hard-fail --exit -u Servo/wptrunner -Z disable-text-aa --output=/tmp/tmpmZ5lHU/ece4d524-2fa2-4bba-977b-0f41fccf79ae http://localhost:8000/css21_dev/html4/abspos-replaced-width-margin-000.htm
(pid:6491) "thread 'LayoutWorker worker 4/4' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src/libcore/result.rs:731"
 1:08.03 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "stack backtrace:"
 1:08.03 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "thread 'ImageCacheThread' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src/libcore/result.rs:731"
 1:08.25 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   1:     0x7f87bf3058ee - sys::backtrace::write::hfe027f56cae344aapps"
 1:08.25 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   2:     0x7f87bf308fd5 - panicking::on_panic::hcd0100c6199f27b1Cax"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   3:     0x7f87bf2f612e - rt::unwind::begin_unwind_inner::h0544b2c664549c40gQw"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   4:     0x7f87bf2f6b1c - rt::unwind::begin_unwind_fmt::h5a900a56c2bf32ccmPw"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   5:     0x7f87bf3089c6 - rust_begin_unwind"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   6:     0x7f87bf33abe4 - panicking::panic_fmt::h205f328075c6fc3cneC"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   7:     0x7f87be711860 - image_cache_task::ImageCacheTask::get_image_if_available::h9c3a7e30df93a50e7mb"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   8:     0x7f87bdf4443d - context::LayoutContext<'a>::get_or_request_image::hc612e50aee3a971fUve"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   9:     0x7f87bdf01ee9 - fragment::ImageFragmentInfo::new::h0bb018c347d3dadcfzj"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  10:     0x7f87bdf01bb9 - construct::FlowConstructor<'a>::build_fragment_for_block::hda65d2b0a5302d44Ufd"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  11:     0x7f87bdf2d376 - construct::FlowConstructor<'a>::build_flow_for_block::hb8907ca9bd1f81eeoBd"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  12:     0x7f87bdf241bc - construct::FlowConstructor<'a>.PostorderNodeMutTraversal::process::h3c16ee2c7d14489fu8d"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  13:     0x7f87be0043c6 - traversal::ConstructFlows<'a>.PostorderDomTraversal::process::h89be43dff5f2fd1b3vw"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  14:     0x7f87be0030ef - parallel::RecalcStyleForNode<'a>.ParallelPreorderDomTraversal::run_parallel::h09612dc432342abeARr"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  15:     0x7f87be004242 - parallel::recalc_style::hfde43ef01c0758820Rr"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  16:     0x7f87bdfa2147 - boxed::F.FnBox<A>::call_box::h12483966634452701337"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  17:     0x7f87bdf739c9 - rt::unwind::try::try_fn::h17634710877808723869"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  18:     0x7f87bf30df88 - rust_try_inner"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  19:     0x7f87bf30df75 - rust_try"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  20:     0x7f87bf303387 - rt::unwind::try::inner_try::h63857c7efe7ef4ac9Lw"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  21:     0x7f87bdf73bd1 - boxed::F.FnBox<A>::call_box::h4738027956056750907"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  22:     0x7f87bf307b11 - sys::thread::Thread::new::thread_start::hb94dd6e8abd80527WVv"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  23:     0x7f87bcb8d181 - start_thread"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  24:     0x7f87bd4a947c - __clone"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  25:                0x0 - <unknown>"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "stack backtrace:"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   1:     0x7f87bf3058ee - sys::backtrace::write::hfe027f56cae344aapps"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   2:     0x7f87bf308fd5 - panicking::on_panic::hcd0100c6199f27b1Cax"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   3:     0x7f87bf2f612e - rt::unwind::begin_unwind_inner::h0544b2c664549c40gQw"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   4:     0x7f87bf2f6b1c - rt::unwind::begin_unwind_fmt::h5a900a56c2bf32ccmPw"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   5:     0x7f87bf3089c6 - rust_begin_unwind"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   6:     0x7f87bf33abe4 - panicking::panic_fmt::h205f328075c6fc3cneC"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   7:     0x7f87be14aefb - image_cache_task::ImageCache::run::heb7c65da5c477772Tqb"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   8:     0x7f87be17548b - boxed::F.FnBox<A>::call_box::h5007534284982225344"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   9:     0x7f87be0ea089 - rt::unwind::try::try_fn::h13260634535012386045"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  10:     0x7f87bf30df88 - rust_try_inner"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  11:     0x7f87bf30df75 - rust_try"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  12:     0x7f87bf303387 - rt::unwind::try::inner_try::h63857c7efe7ef4ac9Lw"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  13:     0x7f87be0ea291 - boxed::F.FnBox<A>::call_box::h3830421486467922678"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  14:     0x7f87bf307b11 - sys::thread::Thread::new::thread_start::hb94dd6e8abd80527WVv"
 1:08.26 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  15:     0x7f87bcb8d181 - start_thread"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  16:     0x7f87bd4a947c - __clone"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  17:                0x0 - <unknown>"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "thread 'ImageCacheThread' panicked at 'assertion failed: libc::close(self.fd) == 0', /home/servo/.cargo/git/checkouts/ipc-channel-d95a23d1f1577bfc/master/platform/linux/mod.rs:96"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "stack backtrace:"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   1:     0x7f87bf3058ee - sys::backtrace::write::hfe027f56cae344aapps"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   2:     0x7f87bf308fd5 - panicking::on_panic::hcd0100c6199f27b1Cax"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   3:     0x7f87bf2f612e - rt::unwind::begin_unwind_inner::h0544b2c664549c40gQw"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   4:     0x7f87bef92261 - rt::unwind::begin_unwind::h8512406236146873166"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   5:     0x7f87bef8c20a - platform::linux::UnixSender.Drop::drop::h69f1598da158384fFHa"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   6:     0x7f87be14f44d - sync::mpsc::Receiver<T>.Drop::drop::h5797335044568620214"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   7:     0x7f87be14ef92 - std..sync..mpsc..Receiver<net_traits..image_cache_task..ImageCacheCommand>::drop.28612::hcf8eee44ff8fc0f1"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   8:     0x7f87be18d38d - image_cache_task..ImageCache::drop.30258::hb035a84455f5f3ec"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "   9:     0x7f87be1759eb - boxed::F.FnBox<A>::call_box::h5007534284982225344"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  10:     0x7f87be0ea089 - rt::unwind::try::try_fn::h13260634535012386045"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  11:     0x7f87bf30df88 - rust_try_inner"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  12:     0x7f87bf30df75 - rust_try"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  13:     0x7f87bf303387 - rt::unwind::try::inner_try::h63857c7efe7ef4ac9Lw"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  14:     0x7f87be0ea291 - boxed::F.FnBox<A>::call_box::h3830421486467922678"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  15:     0x7f87bf307b11 - sys::thread::Thread::new::thread_start::hb94dd6e8abd80527WVv"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  16:     0x7f87bcb8d181 - start_thread"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  17:     0x7f87bd4a947c - __clone"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "  18:                0x0 - <unknown>"
 1:08.27 PROCESS_OUTPUT: Thread-TestrunnerManager-6 (pid:6491) "thread panicked while panicking. aborting."
 1:08.33 CRASH: Thread-TestrunnerManager-6 pid:26602. Test:None. Minidump anaylsed:False. Signature:[/css21_dev/html4/abspos-replaced-width-margin-000.htm]

 1:08.33 TEST_END: Thread-TestrunnerManager-6 CRASH, expected FAIL

@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 25, 2015

Unable to reproduce locally in isolation.

@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 25, 2015

@bors-servo: retry

@bors-servo
Copy link
Contributor

bors-servo commented Jul 25, 2015

Testing commit 53fe692 with merge cf47022...

bors-servo pushed a commit that referenced this pull request Jul 25, 2015
script: Make the `ImageCacheTask` use IPC.

This necessitated getting rid of the boxed trait object that was being
be passed between the script task and the image cache task.

r? @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6597)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 25, 2015

💔 Test failed - linux3

@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 25, 2015

Guess I'll have to run this on a bot. I was unable to reproduce in a full test run as well.

@bors-servo
Copy link
Contributor

bors-servo commented Jul 25, 2015

The latest upstream changes (presumably #6616) made this pull request unmergeable. Please resolve the merge conflicts.

This necessitated getting rid of the boxed trait object that was being
be passed between the script task and the image cache task.
@pcwalton pcwalton force-pushed the pcwalton:image-cache-ipc branch from 53fe692 to 82b53d8 Jul 27, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 27, 2015

@bors-servo: r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 27, 2015

📌 Commit 82b53d8 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 27, 2015

Testing commit 82b53d8 with merge e13ebf7...

bors-servo pushed a commit that referenced this pull request Jul 27, 2015
script: Make the `ImageCacheTask` use IPC.

This necessitated getting rid of the boxed trait object that was being
be passed between the script task and the image cache task.

r? @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6597)
<!-- Reviewable:end -->
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 27, 2015

The FD leaking problem might be the culprit for the test failures.

@bors-servo
Copy link
Contributor

bors-servo commented Jul 27, 2015

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

@bors-servo bors-servo merged commit 82b53d8 into servo:master Jul 27, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@pcwalton pcwalton deleted the pcwalton:image-cache-ipc branch Jul 27, 2015
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

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