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

Fix and optimize query profiling #57095

Merged
merged 2 commits into from Jan 8, 2019

Conversation

Projects
None yet
5 participants
@Zoxc
Copy link
Contributor

Zoxc commented Dec 24, 2018

@Zoxc

This comment has been minimized.

Copy link
Contributor Author

Zoxc commented Dec 24, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 24, 2018

⌛️ Trying commit 7c985be with merge e12bb21...

bors added a commit that referenced this pull request Dec 24, 2018

Auto merge of #57095 - Zoxc:prof-fix, r=<try>
Fix and optimize query profiling

r? @michaelwoerister

cc @wesleywiser
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 24, 2018

☀️ Test successful - status-travis
State: approved= try=True

@Zoxc

This comment has been minimized.

Copy link
Contributor Author

Zoxc commented Dec 24, 2018

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 24, 2018

Success: Queued e12bb21 with parent 94bf2c1, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 24, 2018

Finished benchmarking try commit e12bb21

@michaelwoerister
Copy link
Contributor

michaelwoerister left a comment

Thanks, @Zoxc! Good idea to record query counts in bulk.
r=me with the functions renamed.

@@ -742,6 +734,17 @@ macro_rules! define_queries_inner {
}
}

pub fn record_query_hits(&self, sess: &Session) {

This comment has been minimized.

@michaelwoerister

michaelwoerister Jan 7, 2019

Contributor

This should be called something like record_queries_computed unless I'm misreading something.

pub fn record_query(&mut self, category: ProfileCategory) {
let (hits, total) = *self.data.query_counts.get(category);
self.data.query_counts.set(category, (hits, total + 1));
pub fn record_queries(&mut self, category: ProfileCategory, count: usize) {

This comment has been minimized.

@michaelwoerister

michaelwoerister Jan 7, 2019

Contributor

This should renamed to record_queries_computed too then.

result
} else {
// We could not load a result from the on-disk cache, so
// recompute.

self.sess.profiler(|p| p.start_activity(Q::CATEGORY));

This comment has been minimized.

@michaelwoerister

michaelwoerister Jan 7, 2019

Contributor

Just a remark: There should probably be a third category here for things that could be re-used but are not stored in the cache. Not sure how to best handle this.

self.data.query_counts.set(category, (hits, total + 1));
pub fn record_queries(&mut self, category: ProfileCategory, count: usize) {
let (hits, computed) = *self.data.query_counts.get(category);
self.data.query_counts.set(category, (hits, computed + count as u64));

This comment has been minimized.

@michaelwoerister

michaelwoerister Jan 7, 2019

Contributor

An issue for another PR: This looks like we could get missed updates here (and below) with parallel queries.

@Zoxc Zoxc force-pushed the Zoxc:prof-fix branch from 7c985be to 23c742c Jan 7, 2019

@Zoxc

This comment has been minimized.

Copy link
Contributor Author

Zoxc commented Jan 7, 2019

@bors r=michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 7, 2019

📌 Commit 23c742c has been approved by michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 8, 2019

⌛️ Testing commit 23c742c with merge 9d54812...

bors added a commit that referenced this pull request Jan 8, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 8, 2019

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 9d54812 to master...

@bors bors merged commit 23c742c into rust-lang:master Jan 8, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@Zoxc Zoxc deleted the Zoxc:prof-fix branch Jan 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment