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

Enable several WebRender instances to share a thread pool. #855

Merged
merged 1 commit into from Feb 10, 2017

Conversation

@nical
Copy link
Collaborator

nical commented Feb 9, 2017

This is a simplified version of PR #852 which only enables sharing the thread pool.

It would be worth looking into why ThreadPool isn't Sync and see if we can avoid the mutex.

r? @kvark


This change is Reviewable

@kvark
Copy link
Member

kvark commented Feb 9, 2017

Thanks!
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Feb 9, 2017

📌 Commit 2998915 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Feb 9, 2017

Testing commit 2998915 with merge 8783cd5...

bors-servo added a commit that referenced this pull request Feb 9, 2017
Enable several WebRender instances to share a thread pool.

This is a simplified version of PR #852 which only enables sharing the thread pool.

It would be worth looking into why ThreadPool isn't Sync and see if we can avoid the mutex.

r? @kvark

<!-- 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/855)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 9, 2017

💔 Test failed - status-travis

@kvark
Copy link
Member

kvark commented Feb 9, 2017

@bors-servo retry

  • mac bots are grumpy again
@bors-servo
Copy link
Contributor

bors-servo commented Feb 9, 2017

Testing commit 2998915 with merge 205512b...

bors-servo added a commit that referenced this pull request Feb 9, 2017
Enable several WebRender instances to share a thread pool.

This is a simplified version of PR #852 which only enables sharing the thread pool.

It would be worth looking into why ThreadPool isn't Sync and see if we can avoid the mutex.

r? @kvark

<!-- 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/855)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 9, 2017

💔 Test failed - status-travis

@kvark
Copy link
Member

kvark commented Feb 9, 2017

@bors-servo retry

  • mac bots are still sleepy
@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2017

🔒 Merge conflict

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2017

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

@nical nical force-pushed the nical:thread-pool branch from 2998915 to 807c7a3 Feb 10, 2017
@kvark
Copy link
Member

kvark commented Feb 10, 2017

Ok, let's try again. It's Friday, what can possibly go wrong?
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2017

📌 Commit 807c7a3 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2017

Testing commit 807c7a3 with merge 2cf08ab...

bors-servo added a commit that referenced this pull request Feb 10, 2017
Enable several WebRender instances to share a thread pool.

This is a simplified version of PR #852 which only enables sharing the thread pool.

It would be worth looking into why ThreadPool isn't Sync and see if we can avoid the mutex.

r? @kvark

<!-- 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/855)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2017

💔 Test failed - status-travis

@nical nical mentioned this pull request Feb 10, 2017
@kvark
Copy link
Member

kvark commented Feb 10, 2017

Uh, that's new:

The command "curl -sL https://static.rust-lang.org/rustup.sh -o ~/rust-installer/rustup.sh" failed and exited with 35 during .

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2017

Testing commit 807c7a3 with merge e5435e6...

bors-servo added a commit that referenced this pull request Feb 10, 2017
Enable several WebRender instances to share a thread pool.

This is a simplified version of PR #852 which only enables sharing the thread pool.

It would be worth looking into why ThreadPool isn't Sync and see if we can avoid the mutex.

r? @kvark

<!-- 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/855)
<!-- Reviewable:end -->
@kvark
Copy link
Member

kvark commented Feb 10, 2017

I wish bors didn't have to rebuid the platforms that already succeeded for the same changelist.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2017

☀️ Test successful - status-travis
Approved by: kvark
Pushing e5435e6 to master...

@bors-servo bors-servo merged commit 807c7a3 into servo:master Feb 10, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Feb 20, 2017
Blob image renderer

Here is a simple integration of a VectorImageRenderer abstraction that can be implemented to render vector graphics to an image.

The main ideas are that:
 - The interface should be implementable outside of WebRender in order for gecko to put in there things that WebRender cannot handle yet.
 - The result should be transparently usable as a regular image.
 - The intent is for the renderer to use the same thread pool as the glyph cache (an implementation detail of the vector renderer thanks to PR #855)
 - The interface should be extended to support some sort of sparse/tiled output for very large scrollable vector images.

The wr_sample implements a dummy vector renderer that always outputs a checkerboard pattern. You can have a look and see how it works.

Implementation-wise, vector images are treated like raw images, except that when an image is requested, if it is a vector image the resource cache asks the renderer to paint it asynchronously, and waits for the result at the end of the frame, just before inserting it into the texture cache. I am pretty happy with how simple it turns out (but I guess it will become a lot trickier with zoom support), it took me a few attempts, the first few implementations were a lot more complicated.

The VectorImageRenderer interface needs a bit of work. The request expects a certain size, which will probably not play well with the scaling factor that we should apply when zooming. It also passes an ImageDescriptor but it's not obvious to me whether we want to support anything other than RGBA8 (and maybe A8 for masks) and the stride member is also awkward.
I'm not sure what's best between VectorImageData being a ```Vec<u8>``` or a pointer-sized opaque handle like ExternalEvent (in which case we'd need to figure out who gets to delete the thing).

Feedback very welcome @kvark @glennw @jrmuizel @JerryShih

<!-- 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/858)
<!-- Reviewable:end -->
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

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