1562552 - [mozinfo] Refactor mozinfo into a class

Status: NEW

(Testing :: Mozbase, task, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] (away July 6th - July 19th) from bug 1561293 comment #8)

So my approach here would be to move everything into a class, and create a singleton called mozinfo, which then will be exported. Which means that for every consumer of mozinfo the following change will have to be made:

• import mozinfo

• from mozinfo import mozinfo

With that approach no other changes will be necessary to the code at all.

As Andrew mentioned to me on IRC we will have to be very careful about the import costs. Given that this module is used thousands of times in tree, we cannot accept performance loss. So performance tests will have to be made to ensure that.

Beside that we also have the attrs package vendored in tree. Based on that the to be created class could be made depend on it, and using slots might give us an additional extra performance (even it might not be that much).

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Note that while fixing the is* properties I will also put a fix for sanitize() in-place which is currently broken, and never runs its code, simply because the processor is listed as x86_64 and not universal-x86-x86_64. Maybe the latter value was used in the past...

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1562552 - [mozinfo] Refactor mozinfo into a class. r=ahal,gbrown

Because most of the top-level variables except "info" are getting
exported per value and not per reference, updating the mozinfo data
cannot be reflected by those.

The refactoring into a class will now export a class instance by
reference, which will keep compatibility with the existent API but
requires a different import now:

before: import mozinfo
after:  from mozinfo import mozinfo

Given that this module is used pretty much across the build system
too, further performance improvements have been made by caching
the data, which is used most often.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1562552 - [mozinfo] Bump mozinfo to version 2.0.0, and update all mozinfo dependencies. r=ahal,gbrown

Depends on D36589

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1562552 - [mozinfo] Add support for Android as platform. r=ahal,gbrown

Depends on D36590

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1562552 - Use cached properties of the mozinfo class by consumers. r=ahal,gbrown

This will slightly improve the performance for any code using
mozinfo for determining platform details.

Depends on D36591

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1562552 - Use cached methods for platform checks. r=ahal,gbrown

This further improves the performance of code using mozinfo
to run checks for the current platform.

Depends on D36592

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I actually won't have the time to further dig into the dataclass bits. As such I would like to move this out for a possible follow-up bug.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Here a try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a7eb22d71e980f9e1a2abca6c8aba3e08f3aa55

Andrew and Geoff, would you mind giving early feedback? I would appreciate that given that this patch touches so many different files.

I also wonder which of the mozbase packages we have to immediately release too.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Here another try build with full builds instead of artifact ones but a smaller set of tests, because it's important to know if the build process works here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e8c95d9a69247f406c88a5a277b8da14beca7b4

Comment 10

[Geoff Brown [:gbrown]]

I don't have any significant concerns - lgtm!

Comment 11

[Andrew Halberstadt [:ahal]]

Looks reasonable to me, though I notice you changed a bunch of JS files to use mozinfo.isAndroid etc instead of mozinfo.os == "android". I think in those cases you aren't dealing with an instance of MozInfo so I'm pretty sure those will fail.

I'll probably ask you to add __slots__ to the class as well during a proper review:
http://book.pythontips.com/en/latest/__slots__magic.html

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrew Halberstadt [:ahal] from comment #11)

Looks reasonable to me, though I notice you changed a bunch of JS files to use mozinfo.isAndroid etc instead of mozinfo.os == "android". I think in those cases you aren't dealing with an instance of MozInfo so I'm pretty sure those will fail.

Yes, I reverted those already, and maybe forgot to push the latest state.

I'll probably ask you to add __slots__ to the class as well during a proper review:
http://book.pythontips.com/en/latest/__slots__magic.html

Lets see if I will be able to get this all done until end of this week before I'm away.

Comment 13

[Andrew Halberstadt [:ahal]]

Fwiw implementing slots should be a <2min change, it just involves adding a variable that explicitly lists each attribute that the class has.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Reading through the documentation about slots I can see the benefit in general, but I miss the point for mozinfo. Given that we have a singleton here, which gets shared across all consumers how would that improve the situation a lot here? Maybe it should be profiled...

Given that I don't have the time for that, I will just add __slots__, and maybe get rid of the extra properties (os vs. _os) as I set in my last version. We haven't had a protection for overwriting values before, and we still may not need that.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Note that this patch series shouldn't be landed before the next merge given that we are in a soft-freeze period right now. As such marking as wonfix for Firefox 69.

I will also further check the remaining oranges from the last try build to see if those are related to my changes.

Andrew, I think we also need a good strategy for releasing all the packages to PyPI. Maybe you have an advice. I haven't worked on mozbase packages for a while, so not sure which version restrictions apply to each of the packages.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Actually most of test jobs like cppunit, gTest, and jit were failing because the try build was based on an artifact build. As such I triggered those tests again for the full builds:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e8c95d9a69247f406c88a5a277b8da14beca7b4&searchStr=ship

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Current remaining failures seem to be for Wr jobs like:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e8c95d9a69247f406c88a

The test meta data shows that we should mainly have passed tests on Windows 10 x86_64 with the version 10.0.17134.

Joel, do you know which version of Windows 10 is currently in use by the test machines?

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I also decided to just push another try build which will log the whole mozinfo.info dictionary, so that it will be easier to compare:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d569777024cc71af58ec93e0c311b49d6631b79

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With my patch applied the initial mozinfo.info looks like:

*** [MOZINFO]: defaultdict(<function <lambda> at 0x0000000003B9DCF8>, {'os_version': StringVersion ('10.0'), 'version': '10.0.17134', 'has_sandbox': True, 'bits': 32, 'service_pack': '', 'os': 'win', 'processor': 'x86', 'automation': True})

So it's indeed version 10.0.17134. But as you can see it reports the processor as x86, and not x86_64, which means it doesn't match any of the lines in the manifest. I will have this fixed in a moment.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45a848f9954f7b5907434752c8aae5d6d8ced7b1

Comment 21

[Andrew Halberstadt [:ahal]]

No, I don't really have a release strategy.. Just try to follow semver and only update things that depend on the changes. Tbh it's been years since I've bumped more than one package at a time. Generally I'd only tend to release if some out-of-tree consumer needs it.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sorry, but I wish I would have the time to finish that off. If someone else wants to take it feel free.