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.

Sign up for GitHub

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

Jump to bottom

deps: V8: cherry-pick 597f885 #29367

Closed
wants to merge 1 commit into from
Closed

deps: V8: cherry-pick 597f885 #29367

wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Aug 29, 2019 •
edited
Loading

This backports @schuay's fix in V8 that addresses missing function coverage.

The Bug

This bug only occurred on certain OSes, and manifested itself as follows:

  1. A function's coverage information can be tracked multiple times in memory (I don't believe we've figured out the root cause of this yet).
  2. If you have < N functions in your script file, functions with coverage information are listed first.
  3. as soon as you have > N functions in a script file, sorting swaps and, due to an optimization, all block coverage is dropped for a function (because we see that the function itself has no coverage).

The Fix

We now sort functions before applying the optimization of dropping block coverage.

This is difficult to write tests for, because it appears to be architecture dependent but @shuay has been able to confirm the patch (@shuay I don't suppose I could also get you to test against this branch).

see: #27566

@bcoe
deps: V8: cherry-pick 597f885
f099067 
Original commit message:

    [coverage] Deterministically sort collected shared function infos

    Prior to this CL, collected shared function infos with identical
    source ranges were sorted non-deterministically during coverage
    collection. This lead to non-deterministically incorrectly-reported
    coverage due to an optimization which depended on the sort order later
    on.

    With this CL, we now sort shared function infos by the source range
    *and* call count.

    Bug: v8:6000,v8:9212
    Change-Id: If8bf900727591e71dbd0df621e472a4303f3a353
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1771776
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#63411}

Refs: v8/v8@597f885
@bcoe bcoe added v8 engine Issues and PRs related to the V8 dependency. fast-track PRs that do not need to wait for 48 hours to land. backport-requested-v12.x labels Aug 29, 2019
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 29, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Aug 29, 2019

ci: https://ci.nodejs.org/job/node-test-pull-request/25295/
v8: https://ci.nodejs.org/job/node-test-commit-v8-linux/2484/

@nodejs nodejs deleted a comment from nodejs-github-bot Aug 29, 2019
@nodejs-github-bot
Copy link
Collaborator

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

mhdawson
mhdawson approved these changes Aug 29, 2019
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

bnoordhuis
bnoordhuis approved these changes Aug 29, 2019
View reviewed changes
deps/v8/src/debug/debug-coverage.cc
}
std::sort(sorted.begin(), sorted.end(), CompareSharedFunctionInfo);
std::sort(sorted.begin(), sorted.end());
Copy link
Member

Choose a reason for hiding this comment

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

Is perhaps part of the problem that this should be std::stable_sort(sorted.begin(), sorted.end());?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's a chance that the sorting behavior wouldn't flip/flop if we used stable_sort. But I'm not sure that we know if the "counted" function element is always added first, I like that we're more explicit and bring functions with coverage to the top of the sorting order.

Copy link

Choose a reason for hiding this comment

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

Unfortunately stable_sort wouldn't help. The order of shared function infos on the heap is nondeterministic and likewise the initial (unsorted) order in sorted. A stable sort would just preserve that initial order for "same" elements.

The fix in this change is to also consider the SFI's call count while sorting. As described in the comment, it's somewhat of a hack, but works. The cleaner solution, potentially in the future, would be to remove the optimization to omit "irrelevant" coverage.

bcoe added a commit that referenced this pull request Aug 30, 2019
@bcoe
deps: V8: cherry-pick 597f885
8f33053 
Original commit message:

    [coverage] Deterministically sort collected shared function infos

    Prior to this CL, collected shared function infos with identical
    source ranges were sorted non-deterministically during coverage
    collection. This lead to non-deterministically incorrectly-reported
    coverage due to an optimization which depended on the sort order later
    on.

    With this CL, we now sort shared function infos by the source range
    *and* call count.

    Bug: v8:6000,v8:9212
    Change-Id: If8bf900727591e71dbd0df621e472a4303f3a353
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1771776
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#63411}

Refs: v8/v8@597f885

PR-URL: #29367
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bcoe
Copy link
Contributor Author

bcoe commented Aug 30, 2019

Landed in 8f33053

@bcoe bcoe closed this Aug 30, 2019
@bcoe bcoe deleted the fix-missing-coverage branch August 30, 2019 14:04
@Trott
Copy link
Member

Trott commented Aug 30, 2019

Note that we're supposed to get explicit 👍 for fast-tracking. (In other words, someone approving the PR is not assumed to also be approving fast-tracking. They have to make it explicit.)

Not a big deal here, I don't think, but perhaps a sign that we could use a smaller/simpler set of rules and/or more-automated enforcement. (Right now, I think node-core-utils checks that there's a fast-track label but doesn't really have a way to check for fast-track approval.)

@Trott
Copy link
Member

Trott commented Aug 30, 2019

Also: 🎉!!! I'm excited to see how this impacts coverage reports here and elsewhere.

@bcoe
Copy link
Contributor Author

bcoe commented Aug 30, 2019 •
edited
Loading

@Trott I assumed we were good to go, because I'd added the fast-track label before any approvals rolled in, otherwise I would have solicited 👍

Might be nice to add some GitHub automation around the label.

@bcoe
Copy link
Contributor Author

bcoe commented Aug 30, 2019

but apologies, it is clear from the documentation that I should have also added a comment for folks to upvote.

@Trott
Copy link
Member

Trott commented Aug 30, 2019

@Trott I assumed we were good to go, because I'd added the fast-track label before any approvals rolled in,

Yeah, that assumption is a reasonable one and we should probably consider either changing the rule to accommodate that assumption or else finding a way to have node-core-utils enforce any additional requirements.

schuay
schuay reviewed Sep 2, 2019
View reviewed changes
Copy link

@schuay schuay left a comment

Choose a reason for hiding this comment

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

Backmerge looks good, thanks!

BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
@bcoe @BridgeAR
deps: V8: cherry-pick 597f885
7a4ae1a 
Original commit message:

    [coverage] Deterministically sort collected shared function infos

    Prior to this CL, collected shared function infos with identical
    source ranges were sorted non-deterministically during coverage
    collection. This lead to non-deterministically incorrectly-reported
    coverage due to an optimization which depended on the sort order later
    on.

    With this CL, we now sort shared function infos by the source range
    *and* call count.

    Bug: v8:6000,v8:9212
    Change-Id: If8bf900727591e71dbd0df621e472a4303f3a353
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1771776
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#63411}

Refs: v8/v8@597f885

PR-URL: #29367
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Sep 4, 2019
@bcoe @BridgeAR
deps: V8: cherry-pick 597f885
28ffc9f 
Original commit message:

    [coverage] Deterministically sort collected shared function infos

    Prior to this CL, collected shared function infos with identical
    source ranges were sorted non-deterministically during coverage
    collection. This lead to non-deterministically incorrectly-reported
    coverage due to an optimization which depended on the sort order later
    on.

    With this CL, we now sort shared function infos by the source range
    *and* call count.

    Bug: v8:6000,v8:9212
    Change-Id: If8bf900727591e71dbd0df621e472a4303f3a353
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1771776
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#63411}

Refs: v8/v8@597f885

PR-URL: #29367
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schuay schuay schuay left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@Trott Trott Trott approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

@addaleax addaleax Awaiting requested review from addaleax

@hashseed hashseed Awaiting requested review from hashseed

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@bcoe @nodejs-github-bot @Trott @schuay @bnoordhuis @cjihrig @mhdawson @BridgeAR