Skip to content

test: don't inspect values if not necessary - #22903

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:minimize-inspect-overhead
Closed

test: don't inspect values if not necessary#22903
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:minimize-inspect-overhead

Conversation

@BridgeAR

@BridgeARBridgeAR commented Sep 17, 2018

Copy link
Copy Markdown
Member

The inspection triggered on each assert call eagerly even tough the
assertion was never triggered. That caused significant CPU overhead.

I currently try to figure out what a nice API would look like for lazy custom assert messages but this can go in right away.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@BridgeAR
test: don't inspect values if not necessary
927c162
The inspection triggered on each assert call eagerly even tough the
assertion was never triggered. That caused significant CPU overhead.
@nodejs-github-bot

nodejs-github-bot commented Sep 17, 2018

Copy link
Copy Markdown
Collaborator

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/908/pipeline

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2018
@BridgeARBridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 17, 2018
@BridgeAR

BridgeAR commented Sep 17, 2018

Copy link
Copy Markdown
MemberAuthor

CI https://ci.nodejs.org/job/node-test-pull-request/17241/

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 17, 2018
@Trott

Trott commented Sep 18, 2018•
edited by BridgeAR
Loading

Copy link
Copy Markdown
Member

Resume build: https://ci.nodejs.org/job/node-test-pull-request/17270/ ✔️ (besides arm)

I expect the arm-fanned re-run to fail so that will need a Rebuild, but let's see if this at least clears up the other two red subtasks.

@BridgeAR

BridgeAR commented Sep 18, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

arm rebuild: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3576/ ✔️

@BridgeAR

BridgeAR commented Sep 18, 2018

Copy link
Copy Markdown
MemberAuthor

Landed in 9ccf5c8 🎉

BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 18, 2018
@BridgeAR
test: don't inspect values if not necessary
9ccf5c8
The inspection triggered on each assert call eagerly even tough the
assertion was never triggered. That caused significant CPU overhead.
PR-URL: nodejs#22903
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@addaleax

addaleax commented Sep 18, 2018

Copy link
Copy Markdown
Member

@BridgeAR Just for future reference, from our collaborator guide:

When a pull request is deemed suitable to be fast-tracked, label it with
fast-track. The pull request can be landed once 2 or more Collaborators
approve both the pull request and the fast-tracking request, […]

What most people do is to something like “:+1: this comment if you approve of fast-tracking” to a comment on the PR, and then wait for approvals.

targos pushed a commit that referenced this pull request Sep 18, 2018
@BridgeAR@targos
test: don't inspect values if not necessary
8d5ab42
The inspection triggered on each assert call eagerly even tough the
assertion was never triggered. That caused significant CPU overhead.
PR-URL: #22903
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targostargos mentioned this pull request Sep 18, 2018
Merged
@joyeecheungjoyeecheung mentioned this pull request Sep 18, 2018
Closed
targos pushed a commit that referenced this pull request Sep 19, 2018
@BridgeAR@targos
test: don't inspect values if not necessary
57f2c12
The inspection triggered on each assert call eagerly even tough the
assertion was never triggered. That caused significant CPU overhead.
PR-URL: #22903
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2018
@BridgeAR@targos
test: don't inspect values if not necessary
e580a44
The inspection triggered on each assert call eagerly even tough the
assertion was never triggered. That caused significant CPU overhead.
PR-URL: #22903
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR
BridgeAR deleted the minimize-inspect-overhead branch January 20, 2020 11:38
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@addaleaxaddaleaxaddaleax approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fast-trackPRs that do not need to wait for 48 hours to land.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@BridgeAR@nodejs-github-bot@Trott@addaleax