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

Optimize WebGL function calls #16679

Closed
wants to merge 1 commit into from
Closed

Conversation

@MortimerGoro
Copy link
Contributor

MortimerGoro commented May 1, 2017

I measured the cost of calling some WebGL methods from JavaScript and they are slower compared to Firefox or Chrome. It seems that one of the bottlenecks is the ipc-channel. Current Servo WebGL implementation uses two ipc-channel layers: DOM<=>WebGLPaintThread and WebGLPaintThread<=>Webrender.

This PR replaces the first DOM<=>WebGLPaintThread ipc-channel with a faster mpsc:channel. This doesn't give untrusted content access to the GPU to DOM because the WebGLContexts are still owned by WebRender and the real render messages still sent through the second ipc-channel.

With this change the inmediate cost of simple WebGL method calls are closer to what we see in Firefox or Chrome. Example with measured times on a Desktop linux:

New version, 200 webgl.clearColor calls. Frame times in milliseconds.

0.07886199999938981
0.0779819999997926
0.04238999999961379
0.04007799999999406
0.08114899999964109
0.07670300000063435
0.08235400000012305
0.07708800000000338
0.08082799999920098
0.08204200000000128
0.08140400000047521
0.08121699999992416
0.0797919999995429

Old version, 200 webgl.clearColor calls. Frame times in milliseconds.

0.9031110000000808
0.8622270000000753
0.9009390000001076
0.8202759999999216
0.848363000000063
0.8556140000000596
0.8366560000004029
0.864226000000599
0.8818160000000717
0.8485639999998966
0.9495310000002064
0.8295939999998154
0.8321460000006482

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented May 1, 2017

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/lib.rs, components/constellation/Cargo.toml, components/constellation/constellation.rs
  • @KiChjang: components/script/dom/vrdisplay.rs, components/script/dom/webglframebuffer.rs, components/script/dom/htmlcanvaselement.rs, components/script_traits/lib.rs, components/script_traits/lib.rs and 15 more
  • @fitzgen: components/script/dom/vrdisplay.rs, components/script/dom/webglframebuffer.rs, components/script/dom/htmlcanvaselement.rs, components/script_traits/lib.rs, components/script_traits/lib.rs and 15 more
  • @emilio: components/layout/display_list_builder.rs, components/script/dom/webglframebuffer.rs, components/canvas/webgl_paint_thread.rs, components/script/dom/webglrenderbuffer.rs, components/script/dom/webglprogram.rs and 5 more
@highfive
Copy link

highfive commented May 1, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify layout and script code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member

jdm commented May 1, 2017

@bors-servo
Copy link
Contributor

bors-servo commented May 1, 2017

Trying commit 8b79360 with merge 2e644b3...

bors-servo added a commit that referenced this pull request May 1, 2017
Optimize WebGL function calls

<!-- Please describe your changes on the following line: -->

I measured the cost of calling some WebGL methods from JavaScript and they are slower compared to Firefox or Chrome.  It seems that one of the bottlenecks is the ipc-channel. Current Servo WebGL implementation uses two ipc-channel layers: DOM<=>WebGLPaintThread and WebGLPaintThread<=>Webrender.

This PR replaces the first DOM<=>WebGLPaintThread ipc-channel with a faster mpsc:channel. This doesn't give untrusted content access to the GPU to DOM because the WebGLContexts are still owned by WebRender and the real render messages still sent through the second ipc-channel.

With this change the inmediate cost of simple WebGL method calls are closer to what we see in Firefox or Chrome. Example with measured times on a Desktop linux:

New version, 200 webgl.clearColor calls. Frame times in milliseconds.
```
0.07886199999938981
0.0779819999997926
0.04238999999961379
0.04007799999999406
0.08114899999964109
0.07670300000063435
0.08235400000012305
0.07708800000000338
0.08082799999920098
0.08204200000000128
0.08140400000047521
0.08121699999992416
0.0797919999995429
```

Old version, 200 webgl.clearColor calls. Frame times in milliseconds.

```
0.9031110000000808
0.8622270000000753
0.9009390000001076
0.8202759999999216
0.848363000000063
0.8556140000000596
0.8366560000004029
0.864226000000599
0.8818160000000717
0.8485639999998966
0.9495310000002064
0.8295939999998154
0.8321460000006482
```
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16679)
<!-- Reviewable:end -->
@MortimerGoro MortimerGoro force-pushed the MortimerGoro:webgl_channel branch from 8b79360 to 7385420 May 1, 2017
@bors-servo
Copy link
Contributor

bors-servo commented May 1, 2017

@MortimerGoro: 🔑 Insufficient privileges: and not in try users

1 similar comment
@bors-servo
Copy link
Contributor

bors-servo commented May 1, 2017

@MortimerGoro: 🔑 Insufficient privileges: and not in try users

@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented May 1, 2017

@jdm Could you run bors-servo try again? I pushed a minor change just after you ran it and the build stopped

@emilio
Copy link
Member

emilio commented May 1, 2017

@bors-servo
Copy link
Contributor

bors-servo commented May 1, 2017

Trying commit 7385420 with merge f4dd338...

bors-servo added a commit that referenced this pull request May 1, 2017
Optimize WebGL function calls

<!-- Please describe your changes on the following line: -->

I measured the cost of calling some WebGL methods from JavaScript and they are slower compared to Firefox or Chrome.  It seems that one of the bottlenecks is the ipc-channel. Current Servo WebGL implementation uses two ipc-channel layers: DOM<=>WebGLPaintThread and WebGLPaintThread<=>Webrender.

This PR replaces the first DOM<=>WebGLPaintThread ipc-channel with a faster mpsc:channel. This doesn't give untrusted content access to the GPU to DOM because the WebGLContexts are still owned by WebRender and the real render messages still sent through the second ipc-channel.

With this change the inmediate cost of simple WebGL method calls are closer to what we see in Firefox or Chrome. Example with measured times on a Desktop linux:

New version, 200 webgl.clearColor calls. Frame times in milliseconds.
```
0.07886199999938981
0.0779819999997926
0.04238999999961379
0.04007799999999406
0.08114899999964109
0.07670300000063435
0.08235400000012305
0.07708800000000338
0.08082799999920098
0.08204200000000128
0.08140400000047521
0.08121699999992416
0.0797919999995429
```

Old version, 200 webgl.clearColor calls. Frame times in milliseconds.

```
0.9031110000000808
0.8622270000000753
0.9009390000001076
0.8202759999999216
0.848363000000063
0.8556140000000596
0.8366560000004029
0.864226000000599
0.8818160000000717
0.8485639999998966
0.9495310000002064
0.8295939999998154
0.8321460000006482
```
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

emilio commented May 1, 2017

I believe, btw, that WebGLPaintThreads are supposed to live in the UI process. I'd need to double-check tomorrow, but seems to me that the layer of indirection we could theoretically remove is the WebGLPaintThread <=> WR.

But actually, the paint thread when WR is enabled is just a proxy, so we may be able to avoid creating a WebGLPaintThread entirely when using the WR back-end?

@bors-servo
Copy link
Contributor

bors-servo commented May 1, 2017

💔 Test failed - linux-rel-wpt

@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented May 2, 2017

But actually, the paint thread when WR is enabled is just a proxy, so we may be able to avoid creating a WebGLPaintThread entirely when using the WR back-end?

I thought about removing the WebGLPaintThread too. It seems possible to entirely avoid creating it when WR is enabled. I didn't remove yet because I thought that the thread could help with the ipc-channel serialziation slowness (the slow serialization work is done in background instead of blocking JS).

@MortimerGoro MortimerGoro force-pushed the MortimerGoro:webgl_channel branch from 7385420 to 9633cae May 3, 2017
@highfive highfive removed the S-tests-failed label May 3, 2017
@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented May 3, 2017

Ready for retry. Some tests failed because pixels were upside-down when a webglcanvas is used as source in texImage2D/texSubImage2D. Previously webgl-canvas textImage2D sources were not implemented and all pixels were 0,0,0,0

@emilio
Copy link
Member

emilio commented May 3, 2017

As I said, I think this is conceptually wrong, and the channel we should be avoiding is the WebGLPaintThread <=> WR. @pcwalton can confirm if that's right, since he did the initial multi-process stuff, though.

@bors-servo try

@bors-servo
Copy link
Contributor

bors-servo commented May 3, 2017

Trying commit 9633cae with merge b595f0f...

bors-servo added a commit that referenced this pull request May 3, 2017
Optimize WebGL function calls

<!-- Please describe your changes on the following line: -->

I measured the cost of calling some WebGL methods from JavaScript and they are slower compared to Firefox or Chrome.  It seems that one of the bottlenecks is the ipc-channel. Current Servo WebGL implementation uses two ipc-channel layers: DOM<=>WebGLPaintThread and WebGLPaintThread<=>Webrender.

This PR replaces the first DOM<=>WebGLPaintThread ipc-channel with a faster mpsc:channel. This doesn't give untrusted content access to the GPU to DOM because the WebGLContexts are still owned by WebRender and the real render messages still sent through the second ipc-channel.

With this change the inmediate cost of simple WebGL method calls are closer to what we see in Firefox or Chrome. Example with measured times on a Desktop linux:

New version, 200 webgl.clearColor calls. Frame times in milliseconds.
```
0.07886199999938981
0.0779819999997926
0.04238999999961379
0.04007799999999406
0.08114899999964109
0.07670300000063435
0.08235400000012305
0.07708800000000338
0.08082799999920098
0.08204200000000128
0.08140400000047521
0.08121699999992416
0.0797919999995429
```

Old version, 200 webgl.clearColor calls. Frame times in milliseconds.

```
0.9031110000000808
0.8622270000000753
0.9009390000001076
0.8202759999999216
0.848363000000063
0.8556140000000596
0.8366560000004029
0.864226000000599
0.8818160000000717
0.8485639999998966
0.9495310000002064
0.8295939999998154
0.8321460000006482
```
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

MortimerGoro commented May 3, 2017

As I said, I think this is conceptually wrong, and the channel we should be avoiding is the WebGLPaintThread <=> WR. @pcwalton can confirm if that's right, since he did the initial multi-process stuff, though.

I'd love to get rid of WebGLPaintThread. As I said with this PR the WebGLPaintThread is really only used as a "helper" thread to avoid blocking JS thread with serialization work. I measured the JS raf time in some Three.js demo and it has some impact. If there is a way to improve this I don't mind rewriting the code to remove WebGLPaintThread, for example:

  • I saw that bincode-based IPC landed in webrender, can this help?
  • Batching WebGL calls and serialize/send some of them at once may help
@bors-servo
Copy link
Contributor

bors-servo commented May 4, 2017

💥 Test timed out

@jdm
Copy link
Member

jdm commented May 4, 2017

@wafflespeanut
Copy link
Member

wafflespeanut commented May 11, 2017

@jdm
Copy link
Member

jdm commented May 11, 2017

@bors-servo: retry

@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2017

Trying commit 9633cae with merge bc79baa...

bors-servo added a commit that referenced this pull request May 11, 2017
Optimize WebGL function calls

<!-- Please describe your changes on the following line: -->

I measured the cost of calling some WebGL methods from JavaScript and they are slower compared to Firefox or Chrome.  It seems that one of the bottlenecks is the ipc-channel. Current Servo WebGL implementation uses two ipc-channel layers: DOM<=>WebGLPaintThread and WebGLPaintThread<=>Webrender.

This PR replaces the first DOM<=>WebGLPaintThread ipc-channel with a faster mpsc:channel. This doesn't give untrusted content access to the GPU to DOM because the WebGLContexts are still owned by WebRender and the real render messages still sent through the second ipc-channel.

With this change the inmediate cost of simple WebGL method calls are closer to what we see in Firefox or Chrome. Example with measured times on a Desktop linux:

New version, 200 webgl.clearColor calls. Frame times in milliseconds.
```
0.07886199999938981
0.0779819999997926
0.04238999999961379
0.04007799999999406
0.08114899999964109
0.07670300000063435
0.08235400000012305
0.07708800000000338
0.08082799999920098
0.08204200000000128
0.08140400000047521
0.08121699999992416
0.0797919999995429
```

Old version, 200 webgl.clearColor calls. Frame times in milliseconds.

```
0.9031110000000808
0.8622270000000753
0.9009390000001076
0.8202759999999216
0.848363000000063
0.8556140000000596
0.8366560000004029
0.864226000000599
0.8818160000000717
0.8485639999998966
0.9495310000002064
0.8295939999998154
0.8321460000006482
```
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

bors-servo commented May 11, 2017

💔 Test failed - linux-dev

@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented May 12, 2017

Compiling parking_lot_core v0.2.1
LLVM ERROR: Invalid data was encountered while parsing the file
error: Could not compile `parking_lot_core`.
Build failed, waiting for other jobs to finish...
error: build failed
@jdm
Copy link
Member

jdm commented May 12, 2017

@bors-servo: retry

@bors-servo
Copy link
Contributor

bors-servo commented May 12, 2017

Trying commit 9633cae with merge 55ba4af...

bors-servo added a commit that referenced this pull request May 12, 2017
Optimize WebGL function calls

<!-- Please describe your changes on the following line: -->

I measured the cost of calling some WebGL methods from JavaScript and they are slower compared to Firefox or Chrome.  It seems that one of the bottlenecks is the ipc-channel. Current Servo WebGL implementation uses two ipc-channel layers: DOM<=>WebGLPaintThread and WebGLPaintThread<=>Webrender.

This PR replaces the first DOM<=>WebGLPaintThread ipc-channel with a faster mpsc:channel. This doesn't give untrusted content access to the GPU to DOM because the WebGLContexts are still owned by WebRender and the real render messages still sent through the second ipc-channel.

With this change the inmediate cost of simple WebGL method calls are closer to what we see in Firefox or Chrome. Example with measured times on a Desktop linux:

New version, 200 webgl.clearColor calls. Frame times in milliseconds.
```
0.07886199999938981
0.0779819999997926
0.04238999999961379
0.04007799999999406
0.08114899999964109
0.07670300000063435
0.08235400000012305
0.07708800000000338
0.08082799999920098
0.08204200000000128
0.08140400000047521
0.08121699999992416
0.0797919999995429
```

Old version, 200 webgl.clearColor calls. Frame times in milliseconds.

```
0.9031110000000808
0.8622270000000753
0.9009390000001076
0.8202759999999216
0.848363000000063
0.8556140000000596
0.8366560000004029
0.864226000000599
0.8818160000000717
0.8485639999998966
0.9495310000002064
0.8295939999998154
0.8321460000006482
```
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

bors-servo commented May 12, 2017

💔 Test failed - linux-rel-wpt

@bors-servo
Copy link
Contributor

bors-servo commented May 19, 2017

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

@jdm jdm assigned jdm and unassigned wafflespeanut Aug 1, 2017
@jdm
Copy link
Member

jdm commented Aug 15, 2017

@MortimerGoro Is this separate from #17891, or is this work replaced by that PR?

@MortimerGoro
Copy link
Contributor Author

MortimerGoro commented Aug 15, 2017

@jdm yes, totally replaced. This can be closed.

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.