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

Use atomic reference counting to store images and avoid copying them. #566

Merged
merged 1 commit into from Nov 18, 2016

Conversation

@nical
Copy link
Collaborator

nical commented Nov 17, 2016

We currently clone the bytes of images and glyphs before inserting them in the texture cache, because we need to keep a copy around in case the device is reset or the image is evicted from the cache. We want the images and glyphs to be managed on the backend thread while the buffers need to be read on the renderer thread when they get uploaded.
This PR removes the clone of the bytes by sharing the buffers using atomic reference counting.


This change is Reviewable

@nical nical force-pushed the nical:image-arc branch from 09b03a3 to ba03077 Nov 17, 2016
@nical
Copy link
Collaborator Author

nical commented Nov 17, 2016

Rebased on top of the external image stuff. The fact that we still use bytes.clone() with Arc<Vec<u8>> instead of Vec<u8> with a totally different meaning makes me cringe, but Arc doesn't seem to have an equivalent to clone that doesn't read like expensive allocations (something like add_ref would be nice).

@nical
Copy link
Collaborator Author

nical commented Nov 17, 2016

r? @glennw

@glennw
Copy link
Member

glennw commented Nov 17, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 17, 2016

📌 Commit ba03077 has been approved by glennw

@kvark
kvark approved these changes Nov 17, 2016
@Manishearth
Copy link
Member

Manishearth commented Nov 18, 2016

@bors-servo r+ retry clean

@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2016

💡 This pull request was already approved, no need to approve it again.

@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2016

📌 Commit ba03077 has been approved by Manishearth

@Manishearth
Copy link
Member

Manishearth commented Nov 18, 2016

@Manishearth
Copy link
Member

Manishearth commented Nov 18, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2016

📌 Commit ba03077 has been approved by Manishearth

@Manishearth
Copy link
Member

Manishearth commented Nov 18, 2016

@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2016

💡 This pull request was already approved, no need to approve it again.

@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2016

📌 Commit ba03077 has been approved by glennw

@glennw glennw merged commit e306fdf into servo:master Nov 18, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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

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