X Tutup
Skip to content

feat(benchpress): initial support for firefox#2419

Closed
hankduan wants to merge 2 commits intoangular:masterfrom
hankduan:readPerfProfile
Closed

feat(benchpress): initial support for firefox#2419
hankduan wants to merge 2 commits intoangular:masterfrom
hankduan:readPerfProfile

Conversation

@hankduan
Copy link
Copy Markdown
Contributor

@hankduan hankduan commented Jun 8, 2015

Review on Reviewable

@vsavkin vsavkin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 8, 2015
tbosch
tbosch reviewed Jun 11, 2015
View reviewed changes
modules/benchpress/src/firefox_extension/lib/perf_profile_helper.ts Outdated
Copy link
Copy Markdown
Contributor

@tbosch tbosch Jun 11, 2015

Choose a reason for hiding this comment

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

Can we read the perf profile without writing it to a file?

Copy link
Copy Markdown
Contributor

@tbosch tbosch Jun 11, 2015

Choose a reason for hiding this comment

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

This functionality should all go into a FirefoxDriverExtension, similar to ChromeDriverExtension. However, for this you should change the firefox extension to:

  1. report the profile data without writing it to a file (i.e. directly as result of the JS call).
  2. separate stopping the profiler from reading the profile, as Benchpress will read the profile multiple times (after every iteration and maybe later even in between). I.e. the profile would be started when benchpress creates the FirefoxDriverExtension and not stopped until benchpress is done (or maybe actually never stopped, this is the behavior in Chrome...)

@tbosch tbosch added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 11, 2015
@tbosch tbosch removed their assignment Jun 11, 2015
@hankduan hankduan force-pushed the readPerfProfile branch 2 times, most recently from 547b210 to 4f78096 Compare June 15, 2015 21:34
@hankduan
Copy link
Copy Markdown
Contributor Author

hankduan commented Jun 15, 2015

All comments addressed. PTAL @tbosch

tbosch
tbosch reviewed Jun 15, 2015
View reviewed changes
modules/benchpress/src/firefox_extension/lib/perf_profile_helper.ts Outdated
Copy link
Copy Markdown
Contributor

@tbosch tbosch Jun 15, 2015

Choose a reason for hiding this comment

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

Hank,
the mapping of a FF event to an event that we use in benchpress should not be done in the Firefox extension. The idea is to keep the firefox extension as stable as possible.

Please move the code for EventRecord.categorizeEvent into a new class FirefoxWebdriverExtension, similar and next to ChromeWebDriverExtension.

@hankduan hankduan force-pushed the readPerfProfile branch 2 times, most recently from 2c0f736 to 27c33be Compare June 20, 2015 01:09
@hankduan
Copy link
Copy Markdown
Contributor Author

hankduan commented Jun 20, 2015

PTAL

tbosch
tbosch reviewed Jun 24, 2015
View reviewed changes
modules/benchpress/src/firefox_extension/lib/main.ts Outdated
Copy link
Copy Markdown
Contributor

@tbosch tbosch Jun 24, 2015

Choose a reason for hiding this comment

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

Do we still need this? I thought we are not storing the profile in files any more...

Copy link
Copy Markdown
Contributor Author

@hankduan hankduan Jun 24, 2015

Choose a reason for hiding this comment

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

We don't. I kept it there since it's super useful for debugging purposes.

@tbosch
Copy link
Copy Markdown
Contributor

tbosch commented Jun 24, 2015

And the PR title / commit message seems off.
Maybe something with feat(benchpress): initial support for firefox

@hankduan hankduan changed the title add parser for firefox perf profile feat(benchpress): initial support for firefox Jun 24, 2015
@hankduan hankduan force-pushed the readPerfProfile branch 4 times, most recently from 20a00b5 to 14dae28 Compare June 25, 2015 01:43
@hankduan hankduan force-pushed the readPerfProfile branch 8 times, most recently from 5aac62c to 36c81ec Compare June 25, 2015 19:44
tbosch
tbosch reviewed Jun 25, 2015
View reviewed changes
scripts/ci/test_e2e_js.sh Outdated
Copy link
Copy Markdown
Contributor

@tbosch tbosch Jun 25, 2015

Choose a reason for hiding this comment

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

Can you enable this again?

@tbosch tbosch closed this in 0949a4b Jun 25, 2015
jimthedev pushed a commit to jimthedev/angular that referenced this pull request Jun 30, 2015
@hankduan @jimthedev
feat(benchpress): initial support for firefox
e9a13e5 
Closes angular#2419
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 6, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: benchpress cla: yes state: WIP

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hankduan @tbosch @vsavkin @googlebot
X Tutup