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

Setup optimization target deices/browsers #6722

Merged
merged 7 commits into from
Aug 5, 2022

Conversation

Linchenn
Copy link
Collaborator

@Linchenn Linchenn commented Aug 5, 2022

This PR updates the list of the combinations of devices and browsers. The list currently is used only for our nightly-benchmark, and I will use the devices/browsers in this list as WebGL optimization targets.

Specifically, the list will have 6 combinations on desktop/laptop and 17 combinations on mobile:

  1. Windows
    1.1 Win 11: Chrome, Firefox, Edge
    1.2 Win 7: Firefox

  2. Mac - Monterey: Safari, Chrome

  3. Andriod
    image

  4. iOS
    image

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

Copy link
Collaborator Author

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained


e2e/benchmarks/browserstack-benchmark/preconfigured_browser.json line 33 at r1 (raw file):

      "device": null
    },
    "Windows_7_1": {

I don't know why it originally has this target for win7. Should we remove it?

Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @Linchenn, @mattsoulanille, and @pyu10055)


e2e/benchmarks/browserstack-benchmark/preconfigured_browser.json line 33 at r1 (raw file):

Previously, Linchenn wrote…

I don't know why it originally has this target for win7. Should we remove it?

I think this is to capture the lowest os version that we support. We should add lowest os version test for ios and android too. Can be in a separate PR.

Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @Linchenn and @pyu10055)

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! 3 of 1 approvals obtained (waiting on @Linchenn)


e2e/benchmarks/browserstack-benchmark/index.js line 40 at r3 (raw file):

    base: 'BrowserStack',
    browser: 'chrome',
    browser_version: '15.3',

is 15.3 the latest version of chrome? Or it is for safari? the latest of chrome should b 103+


e2e/benchmarks/browserstack-benchmark/preconfigured_browser.json line 33 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

I think this is to capture the lowest os version that we support. We should add lowest os version test for ios and android too. Can be in a separate PR.

The firefox version is also very low, the latest is around 103+

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @Linchenn)

Copy link
Collaborator Author

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @pyu10055)


e2e/benchmarks/browserstack-benchmark/index.js line 40 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

is 15.3 the latest version of chrome? Or it is for safari? the latest of chrome should b 103+

Updated.


e2e/benchmarks/browserstack-benchmark/preconfigured_browser.json line 33 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

The firefox version is also very low, the latest is around 103+

Done.

@Linchenn Linchenn requested a review from pyu10055 August 5, 2022 20:55
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! 3 of 1 approvals obtained (waiting on @pyu10055)

Copy link
Collaborator Author

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 3 of 1 approvals obtained (waiting on @pyu10055)


e2e/benchmarks/browserstack-benchmark/preconfigured_browser.json line 33 at r1 (raw file):

Previously, Linchenn wrote…

Done.

Thank you Na, Matt and Ping. Just synced with Ping, I will separate this list into two lists in a separate PR: one list contains lowest os version/devices that we support (this will be included in nightly benchmark), while another list contains the most popular devices/browsers (will be included in both nightly benchmark and benchmark for optimization)

@Linchenn Linchenn merged commit 6ee743f into tensorflow:master Aug 5, 2022
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.

4 participants