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

Speed up PolylineBatch tests #7269

Merged
merged 6 commits into from Dec 7, 2018
Merged

Speed up PolylineBatch tests #7269

merged 6 commits into from Dec 7, 2018

Conversation

OmarShehata
Copy link
Contributor

Do not merge yet, opening for feedback.

@likangning93 this does some cleanup to DataSources/StaticGroundPolylinePerMaterialBatch. Initially the tests were timing out, and failing subsequent tests because resources were only getting cleaned up if the test succeeded.

  1. I added an afterEach to ensure any added primitives are removed.
  2. I made the geometry all the tests use much smaller, with a much bigger granularity.
  3. I made all the primitives synchronous.

For 3, I had to tweak the API to accept an option to set the created primitive as synchronous. I noticed that StaticGroundPolylinePerMaterialBatch.prototype.update calls Batch.prototype.update so I added the default value to the former. This should work fine unless there are any instances when Batch.prototype.update is called directly and not through the former.

I don't have before/after timing results because the before takes so long that the tests all timeout on this machine, this should be a significant boost.

@cesium-concierge
Copy link

cesium-concierge commented Nov 16, 2018

Thanks for the pull request @OmarShehata!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@OmarShehata
Copy link
Contributor Author

To clarify, I confirmed that these tests actually fail on computers that are not high-end, and the only reason they pass on Travis I believe is because of:

if (!GroundPolylinePrimitive.isSupported(scene)) {
            // Don't fail if GroundPolylinePrimitive is not supported
            return;
        }

Copy link
Contributor

@likangning93 likangning93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anecdotally I'm seeing this go from ~7 seconds to ~1.5 seconds on a Macbook Pro with i7 4980HQ, so that's pretty awesome! I'll try it on a more potato-like computer soon as well.

Source/DataSources/StaticGroundPolylinePerMaterialBatch.js Outdated Show resolved Hide resolved
@@ -133,7 +135,7 @@ define([

primitive = new GroundPolylinePrimitive({
show : false,
asynchronous : true,
asynchronous : asynchronous,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might not be running into the defined(primitive) && !primitive.ready case below anymore in unit testing. Ideally we'd have a separate spec for that, but this file isn't public facing so if there isn't an elegant way to do that it's not a huge deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just the case where it retries on the next frame right? I don't see a super obvious way to do it and I'm not sure it's worth adding a separate spec for right now.

@OmarShehata
Copy link
Contributor Author

@likangning93 I added your feedback, and all tests are passing locally.

Copy link
Contributor

@likangning93 likangning93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just another couple small things. I also took a quick look at coverage for this file and this PR is actually up a little bit, so no more concerns there.

Do you think it would be a large performance hit to run maybe one spec async instead of sync though?
@mramato would say that these tests don't look very "unit," but as long as we're keeping "in-vivo" specs it makes me a little antsy to only run synchronous specs.

@likangning93
Copy link
Contributor

@OmarShehata bump on a few more things

@OmarShehata
Copy link
Contributor Author

Yes, thanks for the reminder @likangning93 ! It's been on my list. Will get to it later today.

@OmarShehata
Copy link
Contributor Author

@likangning93 I took care of the cleanup issues. I feel like having one test be async would be a big waste, especially if each suite started having at least one async. I feel like that code path could be tested in tests that specifically have something to test with async, but otherwise we should stick with synchronous.

@OmarShehata
Copy link
Contributor Author

@likangning93 this is ready to merge!

@likangning93
Copy link
Contributor

likangning93 commented Dec 7, 2018

Confirmed that these tests finish now instead of timing out on my n2920-based machine, yay!

@likangning93 likangning93 merged commit 0dd5b03 into CesiumGS:master Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants