1981848 - cache manifestparser expression parsing

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[[e-mail] (:bhearsum)]

manifestparser's expression parsing repeatedly tokenizes the same text over and over, and also re-evaluates the same tokenized expression with the same values over and over.

On my local machine I measured the time between "Loading tasks for kind test" and "Generated xxxxx tasks for kind test" (this encompasses all of the time spent running test transforms, which is where this code is called from). I got the following results:
Without the patch: 88s, 91s, 90s
With the patch: 76s, 75s, 75s

I ran a few decision tasks on Try pushes with and without this patch and got the following results:
Without the patch: 106s, 107s, 106s
With the patch: 89s, 94s, 89s

So, clear savings in decision tasks as well, and at least in the case of my machine, about 30 seconds removed from the critical path of try results.

Comment 1

[[e-mail] (:bhearsum)]

Attachment: Bug 1981848: cache test manifest expression parsing r?jmaher!

At two points:

1. The parsing of the raw expression from a manifest
2. The evaluation of the expression with final values

Both of these things are done repeatedly.

On my local machine I measured the time between "Loading tasks for kind test" and "Generated xxxxx tasks for kind test" (this encompasses all of the time spent running test transforms, which is where this code is called from). I got the following results:
Without the patch: 88s, 91s, 90s
With the patch: 76s, 75s, 75s

I ran a few decision tasks on Try pushes with and without this patch and got the following results:
Without the patch: 106s, 107s, 106s
With the patch: 89s, 94s, 89s

So, clear savings in decision tasks as well, and at least in the case of my machine, about 30 seconds removed from the critical path of try results.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/4c80ec1fb5c4
https://hg.mozilla.org/integration/autoland/rev/fd735ea047f9
cache test manifest expression parsing r=jmaher

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/254f5069e10c
https://hg.mozilla.org/integration/autoland/rev/d3b249669ce0
Revert "Bug 1981848: cache test manifest expression parsing r=jmaher" for causing multiple errors AttributeError related.

Comment 4

[Cristian Tuns]

Backed out for causing multiple failures

• Backout link
• Push with failures
• Failure Log
• Failure line: TypeError: unhashable type: 'list'
  [taskcluster:error] exit status 1

https://treeherder.mozilla.org/logviewer?job_id=521601670&repo=autoland
Failure line: AttributeError: module 'functools' has no attribute 'cache'

Comment 5

[[e-mail] (:bhearsum)]

Attachment: Bug 1981848: make mozinfo's StringVersion.__hash__ work r?jmaher!

self.version is a list, which cannot be hashed.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ae5f83e05b4f
https://hg.mozilla.org/integration/autoland/rev/ca392381b16c
cache test manifest expression parsing r=jmaher
https://github.com/mozilla-firefox/firefox/commit/73b8fb64fb7d
https://hg.mozilla.org/integration/autoland/rev/c43e0e97a00d
make mozinfo's StringVersion.__hash__ work r=jmaher

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a0a64fd617be
https://hg.mozilla.org/integration/autoland/rev/7c811cd3bb44
Revert "Bug 1981848: make mozinfo's StringVersion.__hash__ work r=jmaher" for causing multiple perftests failures.

Comment 8

[Serban Stanca [:SerbanS]]

Reverted this because it was causing multiple perftests failures.

• Revert link
• Push with failures
• Failure Log
• Failure line: TypeError: unhashable type: 'list'

Comment 9

[[e-mail] (:bhearsum)]

Attachment: Bug 1981848: mozperftest: don't pass unhashable types as `values` to `active_tests` r?sparky!

In another commit in this stack, we're relying on being able to hash values for some performance wins.

Comment 10

[[e-mail] (:bhearsum)]

(In reply to Serban Stanca [:SerbanS] from comment #8)

Reverted this because it was causing multiple perftests failures.

• Revert link
• Push with failures
• Failure Log
• Failure line: TypeError: unhashable type: 'list'

Thanks; I've got a new patch up that fixes this. I also audited for other calls that be passing unhashable types and couldn't find any others...

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5614e446922c
https://hg.mozilla.org/integration/autoland/rev/fa50019b3efc
cache test manifest expression parsing r=jmaher DONTBUILD
https://github.com/mozilla-firefox/firefox/commit/717306e59e59
https://hg.mozilla.org/integration/autoland/rev/868cd3b14c70
make mozinfo's StringVersion.__hash__ work r=jmaher DONTBUILD
https://github.com/mozilla-firefox/firefox/commit/e05412b0bb40
https://hg.mozilla.org/integration/autoland/rev/99f7057dbf2a
mozperftest: don't pass unhashable types as `values` to `active_tests` r=sparky,mozperftest-reviewers DONTBUILD

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/fa50019b3efc
https://hg.mozilla.org/mozilla-central/rev/868cd3b14c70
https://hg.mozilla.org/mozilla-central/rev/99f7057dbf2a