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

Make ./wpt run [single test] quicker #15137

Open
gsnedders opened this issue Jan 29, 2019 · 10 comments
Open

Make ./wpt run [single test] quicker #15137

gsnedders opened this issue Jan 29, 2019 · 10 comments
Labels
infra priority:backlog wptrunner The automated test runner, commonly called through ./wpt run

Comments

@gsnedders
Copy link
Member

The goal here is to make time ./wpt run --headless --no-pause chrome infrastructure/assumptions/html-elements.html take under 4s on my local laptop (MacBook Pro (Retina, 15-inch, Mid 2014), i7-4870HQ, 16GB RAM, macOS 10.14).

@gsnedders gsnedders added infra wptrunner The automated test runner, commonly called through ./wpt run priority:roadmap labels Jan 29, 2019
@gsnedders gsnedders self-assigned this Jan 29, 2019
@gsnedders
Copy link
Member Author

To give some rough ideas about perf:

cProfile claims that testrunner.run is only 28% of a --no-manifest run. Other significant things are setting up the venv (12%) and requirements (15%), wptrunner.get_loader (34%).

Also looking at VMProf (which seems not to cope with threading locally?! so percentages are in no way comparable), it appears that the more expensive part of wptrunner.get_loader is not the JSON parsing, (manifest.load_and_update is 23%) but constructing all the items (manifest.TypeData.load_all is 55%).

So it seems our biggest enemy is constructing all the manifest item instances, assuming the manifest update takes no time (which is, of course, unrealistic, but it can be much closer to zero, and if we need to redesign the manifest format again we need to know where the bottlenecks are after update).

@jgraham
Copy link
Contributor

jgraham commented Jan 29, 2019

There exists a (some what fragile) optimistation to only construct instances on demand, but we are presumably not using it in this case.

@gsnedders
Copy link
Member Author

There exists a (some what fragile) optimistation to only construct instances on demand, but we are presumably not using it in this case.

I don't see how or where? TestLoader.iter_tests always will, no?

@jgraham
Copy link
Contributor

jgraham commented Jan 30, 2019

Right, if we insist on iterating all tests then obviously we don't use the optimisation. But the TypeData class has the ability to only convert from the JSON representation to the object representation on demand. I imagine in this case we aren't using it because the test matching assumes that we have access to the full list of tests and paths which currently requires us to actually construct all the objects.

@gsnedders
Copy link
Member Author

Vague to-do from the various things I've looked into in the past week:

  • Make sure the lazy item object creation optimisation applies in the jsshell test where it should already apply (cc/ @Ms2ger)
  • Apply the lazy item object creation optimisation when we know what tests we're running (e.g., the single test case)
  • Ensure we don't create objects if we don't need to for a no-op manifest update (this probably means extending the mtime cache)
  • Make it possible to do partial manifest updates (this is currently hard in the reftest case, where paths can cease being a test and become a node in the graph)
  • Consider whether it's worthwhile writing the manifest to disk for partial updates (hard to know what to do here, we could end up with a largely out-of-date manifest and always having to compute the update? maybe just do a full update if the mtime of the manifest is >1 week ago)

@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 6, 2019

@jgraham
Copy link
Contributor

jgraham commented Feb 6, 2019

It could be worth trying doing the serialization on a seperate thread.

@gsnedders
Copy link
Member Author

Ergh, profiling multiprocessing and multithreaded code is a pain.

Doing some stupid wall clock profiling by just deleting code:

  • 2.4s are spent updating the manifest
  • 2.2s are spent installing pip requirements (when that's a no-op and all requirements are already met)

Those two things account for more than half the runtime currently…

For the latter, we could probably try and do something smart like tox does to avoid having to touch pip.

@foolip
Copy link
Member

foolip commented Jun 10, 2019

@gsnedders we've discussed it offline, but can you update this thread with the current plan to make things faster?

@gsnedders
Copy link
Member Author

gsnedders commented Jan 31, 2020

With #16537 having landed:

  • Rewrite manifest include/exclude support #15912 should be much more solvable. I will write a longer comment there.
  • With the include/exclude logic moved into the manifest directly, and with the reftest complexity now gone (i.e., the former _compute_reftests), it should be possible to change Manifest.update to be able to deal with a partial tree. The significant change here is mostly not deleting files that no longer appear in the tree. We also need some way of dealing with manifest.vcs (and looking at mtime caches, etc.) without walking the entire tree. The final part to consider here is how to avoid the cost of serializing all the JSON data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra priority:backlog wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

No branches or pull requests

5 participants