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.

Sign up for GitHub

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

Jump to bottom

Refactor code to simplify manifest items and their creation #15308

Merged
merged 7 commits into from
Feb 26, 2019
Merged

Refactor code to simplify manifest items and their creation #15308

merged 7 commits into from
Feb 26, 2019

Conversation

gsnedders
Copy link
Member

No description provided.

@gsnedders gsnedders force-pushed the manifest-item-refactor branch from 76598af to e3df0ca Compare February 11, 2019 16:25
gsnedders
gsnedders commented Feb 11, 2019
View reviewed changes
tools/manifest/item.py Outdated Show resolved Hide resolved
jgraham
jgraham reviewed Feb 11, 2019
View reviewed changes
tools/manifest/item.py Outdated Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot added the wptrunner The automated test runner, commonly called through ./wpt run label Feb 12, 2019
@gsnedders gsnedders force-pushed the manifest-item-refactor branch 2 times, most recently from bacbe50 to f2ef420 Compare February 12, 2019 20:31
@gsnedders
Copy link
Member Author

So, despite this mostly being done as refactoring to do other work for #15137, this has made the single-test case (time ./wpt run --binary=which chromium --headless --no-pause chrome infrastructure/assumptions/html-elements.html) 20% faster on my desktop computer.

@gsnedders gsnedders force-pushed the manifest-item-refactor branch from f2ef420 to 81bf08f Compare February 12, 2019 20:45
@gsnedders gsnedders force-pushed the manifest-item-refactor branch 3 times, most recently from ad6367e to ff239c7 Compare February 15, 2019 11:46
@gsnedders
Copy link
Member Author

Can't figure out why the test_wpttest.py tests are failing; will need to dig in deeper next week.

@gsnedders gsnedders force-pushed the manifest-item-refactor branch 2 times, most recently from 7fc1c9c to ff641e4 Compare February 18, 2019 14:03
@gsnedders
Copy link
Member Author

The one remaining failure is in test_list_tests_missing_manifest. This appears to be caused by us passing --tests tools/wpt/tests into the wpt run call in that test, and this hella confuses things. We end up generating a manifest for the git repo root but then plenty of other places aren't aware we're doing this.

@gsnedders gsnedders force-pushed the manifest-item-refactor branch from faface3 to 96ecc4c Compare February 19, 2019 18:53
@gsnedders gsnedders mentioned this pull request Feb 21, 2019
Closed
@gsnedders
Copy link
Member Author

gsnedders commented Feb 21, 2019 •
edited
Loading

Given @Hexcles mentioned memory consumption of manifest generation in #12874 (comment):

On Linux/x86_64, on this branch, manifest generation from scratch consumes max 487MB, no-op regeneration 202MB.

master, by way of comparison requires max 1408MB and 290MB respectively.

@Hexcles
Copy link
Member

Hexcles commented Feb 21, 2019

That is definitely promising! I also tested the memory footprint of manifest before (~1.5G from scratch) and it is indeed a problem for folks running tests on VMs (or Taskcluster workers, from forks of this repo) with <=4G memory.

We should also try to free the memory ASAP. How much memory does the manifest take when we are running the tests (without manifest generation)? I assume it'd take take less than generating the manifest, and if so, it'd be great to make sure the delta is freed when the generation is completed.

@gsnedders
Copy link
Member Author

And the commit I've just pushed here makes it go down to max 165MB and 187MB respectively (the delta I presume is caused by it loading the old manifest and therefore having that in memory too).

But this does now mean we aren't holding on to any file beyond the point at which we're directly processing that file.

jgraham
jgraham reviewed Feb 25, 2019
View reviewed changes
Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

THis looks great. Let me double check it doesn't break gecko before approving.

tools/manifest/item.py Show resolved Hide resolved
tools/manifest/item.py Show resolved Hide resolved
@jgraham
Copy link
Contributor

jgraham commented Feb 25, 2019

It seems like this broke gecko by calling git unconditionally https://treeherder.mozilla.org/#/jobs?repo=try&author=james%40hoppipolla.co.uk&collapsedPushes=420652&fromchange=551ebec95f9b6ad9b5ac3d8812ee04b61d9024c3&selectedJob=230362281

jgraham
jgraham requested changes Feb 25, 2019
View reviewed changes
Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

Need to ensure that git isn't required when updating with --work.

@gsnedders
Copy link
Member Author

Need to ensure that git isn't required when updating with --work.

That's fixed now.

@gsnedders gsnedders force-pushed the manifest-item-refactor branch 2 times, most recently from c60bb58 to 22a5eb0 Compare February 26, 2019 16:01
@gsnedders
Remove the EqualTimeChunker
9bd9505 
This hasn't been used by anyone in several years, and trying to fix
the tests as part of the work for web-platform-tests#13507 tends to suggest that the
chunker is itself broken. It is, in many ways, the least useful
chunker as it results in test execution order changing every time the
test execution time metadata is updated, and this leads to further
flakiness.
@gsnedders
Create items from SourceFile with correct url_base
066687a
@gsnedders
Make manifest items leaf nodes with no dependencies
055a9ad
@gsnedders
Get rid of tests for behaviour that's impossible
a4acc6b
@gsnedders
Fix TestharnessTest to have a correct meta_key
d7416f7
@gsnedders
SourceFile always generated RefTestNode, never RefTest
b39d7c6 
In various places we write tests under the assumption that SourceFile
returns the applicable type, which it doesn't. Fix our tests to always
return RefTestNode. This allows us to get rid of a try/except block
that exists purely to handle this case which is unreachable without
mocks.
@gsnedders
new_type can never be falsy, just use an else block
9178dc8
@gsnedders gsnedders force-pushed the manifest-item-refactor branch from 2679ebd to 9178dc8 Compare February 26, 2019 17:59
jgraham
jgraham approved these changes Feb 26, 2019
View reviewed changes
tools/manifest/item.py Show resolved Hide resolved
@gsnedders gsnedders merged commit 89a3ecc into web-platform-tests:master Feb 26, 2019
@gsnedders gsnedders deleted the manifest-item-refactor branch February 26, 2019 18:38
@foolip
Copy link
Member

foolip commented Oct 7, 2019

@gsnedders I've been running a script to regenerate manifests for all tagged commits locally and compare with what can be downloaded from GitHub releases. Starting with merge_pr_15308 there are a lot that have an added slash at the beginning of paths. I'm not sure why that happened, was that ever the intended behavior before, during or after this change?

@gsnedders
Copy link
Member Author

@foolip IIRC the code was always broken, this just exposed the broken codepath more; see also #16125/#16135 and the comments removed in ca972d5 dealing with redundant slashes.

@foolip
Copy link
Member

foolip commented Oct 14, 2019

@gsnedders using "dom/nodes/Document-doctype.html" as a test path that's been around since 2014, it looks like the leading slash was originally not there in the manifest and at some point was added. From which commit should it be there, when doing a full rebuild of the manifest?

@gsnedders
Copy link
Member Author

@foolip not sure; it shouldn't be there (and isn't today).

@foolip
Copy link
Member

foolip commented Oct 15, 2019

@gsnedders alright, I'll work on the assumption that it should never have been there. So far it looks like it's there for some manifests created using an incremental update, but not if you recreate the manifest from scratch on the same commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ms2ger Ms2ger Ms2ger left review comments

@jgraham jgraham jgraham approved these changes

@jugglinmike jugglinmike Awaiting requested review from jugglinmike

Assignees

@jugglinmike jugglinmike

Labels
infra manifest wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gsnedders @Hexcles @jgraham @foolip @Ms2ger @jugglinmike @wpt-pr-bot