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

Conversation

@bcoe
Copy link
Member

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

deps: V8: cherry-pick 597f885
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

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

@Trott
Trott approved these changes Aug 29, 2019
@nodejs-github-bot

This comment has been minimized.

@mhdawson
Copy link
Member

left a comment

LGTM

}
std::sort(sorted.begin(), sorted.end(), CompareSharedFunctionInfo);
std::sort(sorted.begin(), sorted.end());

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Aug 29, 2019

Member

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

This comment has been minimized.

Copy link
@bcoe

bcoe Aug 30, 2019

Author Member

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.

This comment has been minimized.

Copy link
@schuay

schuay Sep 2, 2019

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
deps: V8: cherry-pick 597f885
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

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

Landed in 8f33053

@bcoe bcoe closed this Aug 30, 2019

@bcoe bcoe deleted the bcoe:fix-missing-coverage branch Aug 30, 2019

@Trott

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

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

@bcoe

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

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
Copy link

left a comment

Backmerge looks good, thanks!

BridgeAR added a commit that referenced this pull request Sep 3, 2019
deps: V8: cherry-pick 597f885
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 referenced this pull request Sep 3, 2019
BridgeAR added a commit that referenced this pull request Sep 4, 2019
deps: V8: cherry-pick 597f885
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>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
deps: V8: cherry-pick 597f885
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: nodejs#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>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
deps: V8: cherry-pick 597f885
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: nodejs#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
You can’t perform that action at this time.