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

perf: add js-web-frameworks benchmark #34034

Conversation

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Nov 25, 2019

No description provided.

@googlebot googlebot added the cla: yes label Nov 25, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 25, 2019
@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:gree_table_benchmark branch 4 times, most recently from f586042 to 2ede602 Nov 25, 2019
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review Nov 27, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 28, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 28, 2019
Copy link
Member

IgorMinar left a comment

I pushed a commit with a few fixes + added a bunch of comments. I find the structure of the app to be hard to follow, but I don't understand what constraints you had to write it this way. I suggest separating code into 4 parts:

  • the bootstrap code
  • the component under test
  • the code that trigers various actions
  • the benchpress spec

ts_library(
name = "perf_lib",
testonly = 1,

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 28, 2019

Member

readability improvement:

Suggested change
testonly = 1,
testonly = True,

ng_rollup_bundle(
name = "bundle",
entry_point = ":index_aot.ts",

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 28, 2019

Member

maybe drop "_aot" since we only test in aot - rename to index.ts to keep things simple.

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Nov 28, 2019

Author Member

Yeh, was trying to keep it in-line with all the other existing benchmarks but I think that we should have just index.ts. Renamed it here and opened a follow-up issues to cleanup other benchmarks.


<body>

<h2>Ng2 <a href="https://www.stefankrause.net/wp/?p=504" target="_blank">JS Web Frameworks benchmark</a></h2>

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 28, 2019

Member
Suggested change
<h2>Ng2 <a href="https://www.stefankrause.net/wp/?p=504" target="_blank">JS Web Frameworks benchmark</a></h2>
<h2>Angular <a href="https://www.stefankrause.net/wp/?p=504" target="_blank">JS Web Frameworks benchmark</a></h2>
@NgModule({
imports: [BrowserModule],
declarations: [JsWebFrameworksComponent],
bootstrap: [JsWebFrameworksComponent],

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 28, 2019

Member

this is really hard to follow - the bootstrap sequence goes through three or for different files. can you please separate the ngmodule into a separate file or consider moving it into index.ts?

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Nov 28, 2019

Author Member

Totally, couldn't agree more! The thing is that I did this benchmark so it follows the folder / file structure of all the other benchmarks (otherwise it would be confusing because of inconstancy).

For a long time I wanted to do a bigger cleanup of the existing benchmarks but "there was never time for it" :-/ I've opened #34127 listing all the cleanups I think we should do.

We've got 2 options here:

  1. create this benchmark in the way we want to see the final structure, live with the inconsistency for a while, and then cleanup other benchmarks to match this one;
  2. create this benchmark in the way that it follows structure of the existing benchmarks and then cleanup all benchmarks based on #34127

Personally I would prefer (2) but not feeling too strongly about it!

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Dec 2, 2019

Member

Let's go with #2 for now. Thanks for opening the issue.

modules/benchmarks/src/js-web-frameworks/ng2/init.ts Outdated Show resolved Hide resolved
@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:gree_table_benchmark branch from f894e25 to 52186b8 Nov 28, 2019
@pkozlowski-opensource

This comment has been minimized.

Copy link
Member Author

pkozlowski-opensource commented Nov 28, 2019

Thnx for the review @IgorMinar ! I've addressed smaller issues, but I guess the most important item is:

I find the structure of the app to be hard to follow, but I don't understand what constraints you had to write it this way.

Totally with you! The only "constraint" I've imposed here was "consistency with the existing benchmarks folders / files structure".

I've opened #34127 so we can clean-up things. My personal preference would be to merge this PR so it matches the existing structure and then do across-the-board cleanup as described in #34127.
But we can also make this one "role model" and then cleanup other benchmarks.

WDYT?

In any case we need to coordinate any folder / file moves with the LL setup, if I'm not mistaken?

Copy link
Member

IgorMinar left a comment

Lgtm. Let's refactor the folder structure separately from this change as it will affect LL.

@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Dec 2, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "cla/google" is failing
    pending missing required labels: cla: yes
    pending forbidden labels detected: cla: no
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@IgorMinar IgorMinar added cla: yes and removed cla: no labels Dec 2, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 2, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Dec 2, 2019

Merge-assistance: no g3 impact. This is just a new benchmark

@mhevery mhevery closed this in df6d6e0 Dec 2, 2019
mhevery added a commit that referenced this pull request Dec 2, 2019
@pkozlowski-opensource pkozlowski-opensource mentioned this pull request Dec 5, 2019
1 of 14 tasks complete
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 2, 2020

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 Jan 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.