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

slither.io is awfully slow #10381

Closed
ojab opened this issue Apr 3, 2016 · 5 comments
Closed

slither.io is awfully slow #10381

ojab opened this issue Apr 3, 2016 · 5 comments

Comments

@ojab
Copy link

@ojab ojab commented Apr 3, 2016

Minified testcase:

var fps = 0, lrd_mtm = Date.now();

function oef() {
    if (1E3 < Date.now() - lrd_mtm) {
        console.log("FPS: " + fps);
        fps = 0;
        lrd_mtm = Date.now()
    }
    redraw();
    window.requestAnimationFrame(oef)
};

function redraw() {
    fps++;
    var canvas = document.createElement("canvas");
    var canvas2 = document.createElement("canvas");
    canvas.width = 1000;
    canvas.height = 1000;
    var b = canvas2.getContext("2d");
    b.drawImage(canvas, 0, 0)
};

oef();

getting about 5 FPS on servo release build (with and without WebRender enabled, Mac OS X) and 60 FPS in Firefox/Safari/Chrome.
With small canvas (100x100, for example) I'm getting 60 FPS in servo.

I assume that this issue can be duplicate of #7638/#8311/#8281, but AFAIU they are about servo debug builds.

@jdm jdm added the I-perf-slow label Apr 3, 2016
@jdm
Copy link
Member

@jdm jdm commented Apr 3, 2016

We should measure whether the time is being spent under drawImage or whether requestAnimationFrame isn't giving us frequent-enough callbacks.

@ojab
Copy link
Author

@ojab ojab commented Apr 3, 2016

It's actually drawImage() calls, 1000x1000 canvas takes about 0.160 sec w/ WebRender and 0.15-0.20 sec without.

@jdm
Copy link
Member

@jdm jdm commented Apr 3, 2016

I bet the solution is to switch the implementation of draw_html_canvas_element to use a new Canvas2dMsg which takes a handle to an existing canvas rather than the direct pixels. This will avoid the need to wait on a synchronous read-back operation from the first canvas and serialize the pixels twice.

@nox
Copy link
Member

@nox nox commented Oct 1, 2017

The solution is to not have one additional thread per canvas.

@nox nox added the C-reproduced label Oct 1, 2017
bors-servo added a commit that referenced this issue Apr 23, 2018
Moved Canvas rendering to a single thread.

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

Implements the "Subsequent Steps" part of the [NCSU Canvas rendering project.](https://github.com/servo/servo/wiki/Canvas-rendering-project)

I moved most of the functionality from CanvasPaintThread to CanvasData, so CanvasPaintThread essentially just pulls the info out of the message and calls a method on a particular CanvasData element.

I ran into some awkwardness with the fact that one can only take a single mutable reference from a hashmap, though. DrawImageInOther is not really possible to do with only one reference at a time, so it awkwardly still lives in CanvasPaintThread, basically. I also would've preferred to take the reference at the top as soon as I get the CanvasId, since that looks cleaner than having them all start with "canvas_paint_thread.canvases.get_mut[&canvas_id].unwrap()" but that makes trying to take the second reference for DrawImageInOther fail to compile. I'm definitely open to suggestions on how to make that less gross.

The timed single-canvas drawing improved in performance from around ~2.2ms to around ~1.7ms. Slither.io runs better and doesn't crash, but I'm not having it crash on my copy from before these changes, so I don't know if that's new behavior or not.

---
<!-- 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 build-geckolib` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13879 and #10381.

<!-- Either: -->
- [X] There are tests for these changes (added in the initial steps)

<!-- 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/20680)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Apr 23, 2018
Moved Canvas rendering to a single thread.

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

Implements the "Subsequent Steps" part of the [NCSU Canvas rendering project.](https://github.com/servo/servo/wiki/Canvas-rendering-project)

I moved most of the functionality from CanvasPaintThread to CanvasData, so CanvasPaintThread essentially just pulls the info out of the message and calls a method on a particular CanvasData element.

I ran into some awkwardness with the fact that one can only take a single mutable reference from a hashmap, though. DrawImageInOther is not really possible to do with only one reference at a time, so it awkwardly still lives in CanvasPaintThread, basically. I also would've preferred to take the reference at the top as soon as I get the CanvasId, since that looks cleaner than having them all start with "canvas_paint_thread.canvases.get_mut[&canvas_id].unwrap()" but that makes trying to take the second reference for DrawImageInOther fail to compile. I'm definitely open to suggestions on how to make that less gross.

The timed single-canvas drawing improved in performance from around ~2.2ms to around ~1.7ms. Slither.io runs better and doesn't crash, but I'm not having it crash on my copy from before these changes, so I don't know if that's new behavior or not.

---
<!-- 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 build-geckolib` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13879 and #10381.

<!-- Either: -->
- [X] There are tests for these changes (added in the initial steps)

<!-- 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/20680)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Apr 24, 2018
Moved Canvas rendering to a single thread.

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

Implements the "Subsequent Steps" part of the [NCSU Canvas rendering project.](https://github.com/servo/servo/wiki/Canvas-rendering-project)

I moved most of the functionality from CanvasPaintThread to CanvasData, so CanvasPaintThread essentially just pulls the info out of the message and calls a method on a particular CanvasData element.

I ran into some awkwardness with the fact that one can only take a single mutable reference from a hashmap, though. DrawImageInOther is not really possible to do with only one reference at a time, so it awkwardly still lives in CanvasPaintThread, basically. I also would've preferred to take the reference at the top as soon as I get the CanvasId, since that looks cleaner than having them all start with "canvas_paint_thread.canvases.get_mut[&canvas_id].unwrap()" but that makes trying to take the second reference for DrawImageInOther fail to compile. I'm definitely open to suggestions on how to make that less gross.

The timed single-canvas drawing improved in performance from around ~2.2ms to around ~1.7ms. Slither.io runs better and doesn't crash, but I'm not having it crash on my copy from before these changes, so I don't know if that's new behavior or not.

---
<!-- 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 build-geckolib` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13879 and #10381.

<!-- Either: -->
- [X] There are tests for these changes (added in the initial steps)

<!-- 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/20680)
<!-- Reviewable:end -->
@atouchet
Copy link
Contributor

@atouchet atouchet commented Apr 24, 2018

This was addressed by #20680.

@atouchet atouchet closed this Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.