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

deps: V8: cherry-pick 597f885 #29367

Closed
wants to merge 1 commit into from
Closed

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Aug 29, 2019

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

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 <[email protected]>
    Commit-Queue: Jakob Gruber <[email protected]>
    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

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

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

}
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
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 <[email protected]>
    Commit-Queue: Jakob Gruber <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#63411}

Refs: v8/v8@597f885

PR-URL: #29367
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@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

@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.

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
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 <[email protected]>
    Commit-Queue: Jakob Gruber <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#63411}

Refs: v8/v8@597f885

PR-URL: #29367
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
BridgeAR pushed a commit that referenced this pull request Sep 4, 2019
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 <[email protected]>
    Commit-Queue: Jakob Gruber <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#63411}

Refs: v8/v8@597f885

PR-URL: #29367
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

8 participants