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

Introduce SerializedDisplayItem to reduce mem copies on DL construction #3361

Merged
merged 1 commit into from Nov 27, 2018

Conversation

kvark
Copy link
Member

@kvark kvark commented Nov 27, 2018

This PR addresses the part of #3358 about DL construction: instead of constructing the display items and associated data and passing through the serializer by value, we just pass the references around, which guarantees that no extra copies are made.

TODO:

  • verify that the copies are gone (need the tool hosted/opened somewhere)
  • Gecko try push with talos jobs

This change is Reviewable

@kvark kvark requested a review from jrmuizel November 27, 2018 18:17
@kvark
Copy link
Member Author

kvark commented Nov 27, 2018

@kvark kvark changed the title Introduce SerializedDisplayItem to reduce memc opies on DL construction Introduce SerializedDisplayItem to reduce mem copies on DL construction Nov 27, 2018
@kvark
Copy link
Member Author

kvark commented Nov 27, 2018

With @jrmuizel help to set up the tool, it looks like the item copies are all gone for good 🎉 .
The old log starts with:

memcpy of 176 in _ZN13webrender_api12display_list18DisplayListBuilder9push_item17hc51010e3883501b0E @ 
  push_item webrender_api/src/display_list.rs:971
memcpy of 176 in _ZN13webrender_api12display_list18DisplayListBuilder9push_item17hc51010e3883501b0E @ 
  push_item webrender_api/src/display_list.rs:970
memcpy of 176 in _ZN13webrender_api12display_list18DisplayListBuilder31push_item_with_clip_scroll_info17ha854a06e3f76f0daE @ 
  push_item_with_clip_scroll_info webrender_api/src/display_list.rs:987
memcpy of 176 in _ZN13webrender_api12display_list18DisplayListBuilder19push_new_empty_item17hd4bea4fefd9abd2fE @ 
  push_new_empty_item webrender_api/src/display_list.rs:999
memcpy of 168 in _ZN13webrender_api12display_list18DisplayListBuilder15push_clear_rect17h117fa57bbf4055ffE @ 
  push_item webrender_api/src/display_list.rs:970
  push_clear_rect webrender_api/src/display_list.rs:1057
memcpy of 152 in _ZN13webrender_api12display_list18DisplayListBuilder9push_rect17h765bb472db42b8caE @ 
  push_item webrender_api/src/display_list.rs:970
  push_rect webrender_api/src/display_list.rs:1053
memcpy of 146 in _ZN13webrender_api12display_list18DisplayListBuilder9push_line17h7ecefb04d4b38bbfE @ 
  push_item webrender_api/src/display_list.rs:970

The new log doesn't have any of those:

memcpy of 144 in _ZN13webrender_api3api9RenderApi13report_memory17h8993ae29f9c6cd14E @ 
  new<std::sync::mpsc::oneshot::Packet<webrender_api::api::MemoryReport>> liballoc/sync.rs:292
  channel<webrender_api::api::MemoryReport> libstd/sync/mpsc/mod.rs:725
  msg_channel<webrender_api::api::MemoryReport> webrender_api/src/channel_mpsc.rs:62
  report_memory webrender_api/src/api.rs:1099
memcpy of 136 in _ZN13webrender_api3api9RenderApi13report_memory17h8993ae29f9c6cd14E @ 
  recv<webrender_api::api::MemoryReport> libstd/sync/mpsc/shared.rs:245
  recv<webrender_api::api::MemoryReport> libstd/sync/mpsc/mod.rs:1218
  recv<webrender_api::api::MemoryReport> webrender_api/src/channel_mpsc.rs:41
  report_memory webrender_api/src/api.rs:1101
memcpy of 136 in _ZN13webrender_api3api9RenderApi13report_memory17h8993ae29f9c6cd14E @ 
  recv<webrender_api::api::MemoryReport> libstd/sync/mpsc/shared.rs:244
  recv<webrender_api::api::MemoryReport> libstd/sync/mpsc/mod.rs:1218
  recv<webrender_api::api::MemoryReport> webrender_api/src/channel_mpsc.rs:41
  report_memory webrender_api/src/api.rs:1101
memcpy of 136 in _ZN13webrender_api3api9RenderApi13report_memory17h8993ae29f9c6cd14E @ 
  recv<webrender_api::api::MemoryReport> libstd/sync/mpsc/stream.rs:224
  recv<webrender_api::api::MemoryReport> libstd/sync/mpsc/mod.rs:1210
  recv<webrender_api::api::MemoryReport> webrender_api/src/channel_mpsc.rs:41
  report_memory webrender_api/src/api.rs:1101
memcpy of 136 in _ZN13webrender_api3api9RenderApi13report_memory17h8993ae29f9c6cd14E @ 
  recv<webrender_api::api::MemoryReport> libstd/sync/mpsc/stream.rs:219
  recv<webrender_api::api::MemoryReport> libstd/sync/mpsc/mod.rs:1210
  recv<webrender_api::api::MemoryReport> webrender_api/src/channel_mpsc.rs:41

@kvark
Copy link
Member Author

kvark commented Nov 27, 2018

@jrmuizel r? this is ready, I believe, but you need to double check if this makes sense. The memcopies are gone, and Gecko try is green. I'm not seeing a significant difference in Talos, but maybe I'm looking at the wrong thing :)

@gw3583
Copy link
Contributor

gw3583 commented Nov 27, 2018

dl_mutate in g4 talos doesn't seem to have changed much, like you said [1].

But it seems like a worthwhile change to me, will wait to see what @jrmuizel thinks.

[1] https://treeherder.mozilla.org/perf.html#/graphs?series=try,1660472,1,1&selected=try,1660472,408314,654401468

@jrmuizel
Copy link
Collaborator

I think this is fine to take now. It moves the unwrap() back between SpecificDisplayItem construction and serialization which reduces our chances of inlining serialization and constant folding the SpecificDisplayItem into that code. However, because of rust-lang/rust#54878 we're not in a great position to achieve this right now.

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit f72bf26 has been approved by jrmuizel

@bors-servo
Copy link
Contributor

⌛ Testing commit f72bf26 with merge 323b7ea...

bors-servo pushed a commit that referenced this pull request Nov 27, 2018
Introduce SerializedDisplayItem to reduce mem copies on DL construction

This PR addresses the part of #3358 about DL construction: instead of constructing the display items and associated data and passing through the serializer by value, we just pass the references around, which guarantees that no extra copies are made.

TODO:
- [x] verify that the copies are gone (need the tool hosted/opened somewhere)
- [x] Gecko try push with talos jobs

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3361)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: jrmuizel
Pushing 323b7ea to master...

@bors-servo bors-servo merged commit f72bf26 into servo:master Nov 27, 2018
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 29, 2018
…67e04913e9fb (WR PR #3361). r=kats

servo/webrender#3361

Differential Revision: https://phabricator.services.mozilla.com/D13246

--HG--
extra : moz-landing-system : lando
jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Nov 29, 2018
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…67e04913e9fb (WR PR #3361). r=kats

servo/webrender#3361

Differential Revision: https://phabricator.services.mozilla.com/D13246

UltraBlame original commit: 3dcdcddd94ff74e05fb4793e970433772ac99098
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…67e04913e9fb (WR PR #3361). r=kats

servo/webrender#3361

Differential Revision: https://phabricator.services.mozilla.com/D13246

UltraBlame original commit: 3dcdcddd94ff74e05fb4793e970433772ac99098
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…67e04913e9fb (WR PR #3361). r=kats

servo/webrender#3361

Differential Revision: https://phabricator.services.mozilla.com/D13246

UltraBlame original commit: 3dcdcddd94ff74e05fb4793e970433772ac99098
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants