Skip to content

Conversation

@szepeviktor
Copy link

Inspired by @westonruter

@gemini-code-assist
Copy link

Summary of Changes

Hello @szepeviktor, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request updates the run.sh script to incorporate Fast 4G network emulation into the web vitals benchmarking process. By adding a specific network condition flag, the benchmarks will now run under conditions simulating a Fast 4G connection, providing more realistic performance metrics for web performance analysis.

Highlights

  • Network Emulation: The benchmark-web-vitals commands in run.sh now explicitly include the -c "Fast 4G" flag to emulate a Fast 4G network during benchmarking.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces network emulation for the Web Vitals benchmark, setting the condition to 'Fast 4G'. This is a valuable addition for more realistic performance measurements. My review includes a suggestion to use a variable for the network condition to make the script more flexible and configurable.

Comment on lines +109 to +110
npm run research --silent -- benchmark-web-vitals -u http://localhost:8881/ -n 20 -c "Fast 4G" -p -o csv > before.csv
npm run research --silent -- benchmark-web-vitals -u http://localhost:8891/ -n 20 -c "Fast 4G" -p -o csv > after.csv

Choose a reason for hiding this comment

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

medium

To improve the script's flexibility, it's better to use a variable for the network condition instead of a hardcoded string. This allows for easier configuration of the benchmark without modifying the script. The suggested change introduces a CONNECTION variable with a default value of 'Fast 4G', so it can be overridden from the environment if needed.

Suggested change
npm run research --silent -- benchmark-web-vitals -u http://localhost:8881/ -n 20 -c "Fast 4G" -p -o csv > before.csv
npm run research --silent -- benchmark-web-vitals -u http://localhost:8891/ -n 20 -c "Fast 4G" -p -o csv > after.csv
npm run research --silent -- benchmark-web-vitals -u http://localhost:8881/ -n 20 -c "${CONNECTION:-Fast 4G}" -p -o csv > before.csv
npm run research --silent -- benchmark-web-vitals -u http://localhost:8891/ -n 20 -c "${CONNECTION:-Fast 4G}" -p -o csv > after.csv

@westonruter
Copy link
Contributor

Let's have the connection be provided via workflow inputs as well:

It should be a dropdown allowing the choice from among these options: "none", "Slow 3G", "Fast 3G", "Slow 4G", "Fast 4G", "broadband". Maybe it would make sense to default to "Fast 4G".

There are other parameters which would be great to add as well, including CPU throttling and device emulation: https://github.com/GoogleChromeLabs/wpp-research/tree/main/cli#benchmark-web-vitals

These can be done later though.

@westonruter
Copy link
Contributor

I just saw (and remembered) that the GitHub workflow actually does emulate network conditions, specifically for broadband:

npm run research --silent -- benchmark-web-vitals -u http://localhost:8881/ -n 100 -c "broadband" -w 1280x720 -p -v -o csv > before.csv
npm run research --silent -- benchmark-web-vitals -u http://localhost:8891/ -n 100 -c "broadband" -w 1280x720 -p -v -o csv > after.csv

So this means that these results are already throttled, although not to Fast 4G: https://github.com/swissspidy/compare-wp-performance/actions/runs/18974104811

See also :

It was switched from Fast 4G to broadband in a83cf9e because it was really slow.

As noted in #65:

Tests are now super slow with this added throttling.. so maybe it should be opt-in instead?

The opt-in can be via the inputs. I think it would also make sense to have the iteration count be an option. Currently it is 100, but there could be an input to pick something like 20 instead when there is network throttling.

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.

2 participants