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

Reduce data copies during internation #3366

Merged
merged 2 commits into from Nov 30, 2018

Conversation

Projects
None yet
4 participants
@kvark
Copy link
Member

kvark commented Nov 28, 2018

Excessive copying in fn intern() is something we noticed yesterday with @jrmuizel .
This PR attempts to improve them in two ways (each in a separate commit):

  1. reduce the Update enum size by moving the data out
  2. adopt entry-like API to accommodate the common pattern of if index == v.len() { v.push(xxx) } else { v[index] = xxx; } without panics between element construction and actual assignment. It builds upon the VecHelper work of #3362.

This change is Reviewable

@kvark kvark requested review from jrmuizel and gw3583 Nov 28, 2018

@gw3583

gw3583 approved these changes Nov 28, 2018

Copy link
Collaborator

gw3583 left a comment

Should we confirm this is a talos win before merging?

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Nov 28, 2018

@gw3583 yep, will do

Also need to confirm with the playground, since in this case (unlike with Allocation) we still have a branch separating an item constructor from the target location, even though it's not a panicing one.

@kvark kvark force-pushed the kvark:intern-copy branch from 04b1c60 to db16047 Nov 28, 2018

@kvark kvark changed the title Reduce data copies during internation [WIP] Reduce data copies during internation Nov 28, 2018

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Nov 29, 2018

Interestingly, a Playground experiment shows a reduction of LLVM copy calls from 4 to 0 when using this change. Seems pretty ridiculous, you are encouraged to play with it and maybe tell what I'm doing wrong :)

@jrmuizel

This comment has been minimized.

Copy link
Contributor

jrmuizel commented Nov 29, 2018

Here's what my tool can tell us about where the copies are coming from:

memcpy of 402 in _ZN5entry3foo17h9b5dcf419aa0590cE @
  foo entry.rs:78
memcpy of 402 in _ZN5entry3foo17h9b5dcf419aa0590cE @
  write<entry::Thing> /rustc/400c2bc5ed292f77c49693320f4eda37bb375e90/src/libcore/ptr.rs:736
  push<entry::Thing> /rustc/400c2bc5ed292f77c49693320f4eda37bb375e90/src/liballoc/vec.rs:1003
  foo entry.rs:78
memcpy of 402 in _ZN5entry3foo17h9b5dcf419aa0590cE @
  foo entry.rs:80
memcpy of 402 in _ZN5entry3foo17h9b5dcf419aa0590cE @
  foo entry.rs:80

4 copies does seem excessive, but it is facts.

@kvark kvark changed the title [WIP] Reduce data copies during internation Reduce data copies during internation Nov 29, 2018

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Nov 29, 2018

@jrmuizel thanks for confirmation! It's going to be a big win then :)
I'm currently having trouble getting sane talos comparisons, but I'm fairly sure this is safe to land (given the playground results so far).
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=214670152&revision=951e31ce0aa13802a0b12211f85013f57e99dad0

@jrmuizel

This comment has been minimized.

Copy link
Contributor

jrmuizel commented Nov 29, 2018

It would be worth comparing before/after on webrender.ll. You should be able to avoid the verification errors by using rust nightly. (That fixed it for me)

@kvark kvark changed the title Reduce data copies during internation [WIP] Reduce data copies during internation Nov 30, 2018

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Nov 30, 2018

@jrmuizel I'm not seeing as good of a picture as in playground (and yes, nightly works). The change of a first commit strips away a few larger memcopies, but the entry(index).set(value) patter doesn't appear to be helping in real world our case. Would be good to understand why playground results are different.

Edit: here are the dumps: memcpy-logs.zip

@jrmuizel

This comment has been minimized.

Copy link
Contributor

jrmuizel commented Nov 30, 2018

I suspect we're not seeing the improvement we expect because of rust-lang/rust#56191. @nikic has a patch to llvm that might improve things here.

@kvark kvark changed the title [WIP] Reduce data copies during internation Reduce data copies during internation Nov 30, 2018

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Nov 30, 2018

Let's not get this rot for too long.
@bors-servo r=gw3583

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 30, 2018

📌 Commit db16047 has been approved by gw3583

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 30, 2018

⌛️ Testing commit db16047 with merge 1619d94...

bors-servo added a commit that referenced this pull request Nov 30, 2018

Auto merge of #3366 - kvark:intern-copy, r=gw3583
Reduce data copies during internation

Excessive copying in `fn intern()` is something we noticed yesterday with @jrmuizel .
This PR attempts to improve them in two ways (each in a separate commit):
  1. reduce the `Update` enum size by moving the data out
  2. adopt entry-like API to accommodate the common pattern of `if index == v.len() { v.push(xxx) } else { v[index] = xxx; }` without panics between element construction and actual assignment. It builds upon the `VecHelper` work of #3362.

<!-- 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/3366)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 30, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: gw3583
Pushing 1619d94 to master...

@bors-servo bors-servo merged commit db16047 into servo:master Nov 30, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 2, 2018

Bug 1511661 - Update webrender to commit 1619d945e853db14a9d62ed75dce…
…7216ff3cdbc2 (WR PR #3366). r=kats

servo/webrender#3366

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

--HG--
extra : moz-landing-system : lando

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 2, 2018

@kvark kvark deleted the kvark:intern-copy branch Dec 11, 2018

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Dec 11, 2018

Got 5% improvement in dl_mutate from that: https://treeherder.mozilla.org/perf.html#/alerts?id=18030
🎉

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