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

Use VMScriptRef to identify VM scripts, not URI #216

Merged
merged 1 commit into from Oct 19, 2018

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Jan 5, 2018

Previously, coverage collection assumed that a script could be uniquely
identified by URI, which is not a valid assumption. For example, when a
part is loaded via two libraries, both of which are loaded by an
isolate, the VM will track these are two scripts that map to the same
URI.

During collection, we now track each script by its (unique)
VMScriptRef. This ensures we lookup the correct script when computing
the affected line for each hit token. The hitmap remains URI based,
since in the end, we want a single, unified set of line->hitCount
mappings per script.

Fixes #194

@cbracken
Copy link
Member Author

cbracken commented Jan 5, 2018

/cc @amirh

@yjbanov
Copy link

yjbanov commented Oct 19, 2018

lgtm

Previously, coverage collection assumed that a script could be uniquely
identified by URI, which is not a valid assumption. For example, when a
part is loaded via two libraries, both of which are loaded by an
isolate, the VM will track these are two scripts that map to the same
URI.

During collection, we now track each script by its (unique)
VMScriptRef. This ensures we lookup the correct script when computing
the affected line for each hit token. The hitmap remains URI based,
since in the end, we want a single, unified set of line->hitCount
mappings per script.

Fixes dart-lang#194
@cbracken cbracken merged commit 8ca3f6e into dart-lang:master Oct 19, 2018
@cbracken cbracken deleted the use-scriptref-not-uri branch October 19, 2018 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants