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

benchmark: fix EventTarget benchmark #34004

Closed

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Jun 21, 2020

Fixes: #33782

Instead of removing the benchmark completely, I've opted to simply tweak it as little as possible so that it no longer crashes.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@mscdex mscdex added benchmark Issues and PRs related to the benchmark subsystem. eventtarget Issues and PRs related to the EventTarget implementation. labels Jun 21, 2020
@nodejs-github-bot nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Jun 21, 2020
@nodejs-github-bot
Copy link
Collaborator

@mscdex
Copy link
Contributor Author

mscdex commented Jun 21, 2020

On a related note, is there a reason why we aren't running make test-benchmark in CI to prevent issues like this? I thought benchmark tests used to be run along with all of the normal tests?

/cc @nodejs/collaborators

@Trott
Copy link
Member

Trott commented Jun 21, 2020

On a related note, is there a reason why we aren't running make test-benchmark in CI to prevent issues like this? I thought benchmark tests used to be run along with all of the normal tests?

/cc @nodejs/collaborators

People complained that they took too long and timed out too much, so we moved them to node-daily-master. I wouldn't be opposed to adding them back to regular CI runs if we can get regular CI to be less unreliable. The current situation is pretty bad.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
Copy link
Member

I suggest to run the benchmark tests on our CI but not locally.

@mscdex
Copy link
Contributor Author

mscdex commented Jun 21, 2020

People complained that they took too long and timed out too much, so we moved them to node-daily-master. I wouldn't be opposed to adding them back to regular CI runs if we can get regular CI to be less unreliable. The current situation is pretty bad.

As far as I can tell, the node-daily-master CI job isn't currently failing on the EventTarget benchmark like it should be. Does anyone regularly check/monitor the results of node-daily-master?

@puzpuzpuz puzpuzpuz self-requested a review June 22, 2020 08:50
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mscdex
Copy link
Contributor Author

mscdex commented Jun 22, 2020

@jasnell
Copy link
Member

jasnell commented Jun 22, 2020

@mscdex ... I pulled this commit into #34015 so that we can land a batch of the eventtarget related items at once. Do you mind if we close this one in favor of that?

@Trott
Copy link
Member

Trott commented Jun 24, 2020

As far as I can tell, the node-daily-master CI job isn't currently failing on the EventTarget benchmark like it should be. Does anyone regularly check/monitor the results of node-daily-master?

I check it every day. (Sam Roberts did for a while too, but I think he stopped checking a week or three ago.) However I have not had the time to follow up on the various issues. I just see what's failing, feel a pang of guilt that I don't have time to dig in on it right now, and move on. It's my once-a-day guilt-shot.

@mscdex mscdex closed this Jun 24, 2020
@mscdex mscdex deleted the benchmark-fix-events-eventtarget branch July 26, 2020 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. events Issues and PRs related to the events subsystem / EventEmitter. eventtarget Issues and PRs related to the EventTarget implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

benchmark: events/eventtarget throws
7 participants