-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat(difs): Add debug status to dif candidates info #316
Conversation
Instructions for snapshot changesSentry runs a symbolicator integration test suite located at Follow these steps to update snapshots in Sentry:
|
} | ||
} | ||
|
||
impl From<Vec<ObjectCandidate>> for AllObjectCandidates { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe IntoIterator<ObjectCandidate>
would be a better bound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way this can move the Vec rather than re-create it. And this happens to already be created as a Vec, so I don't think there's anything to be gained from being more generic here.
Maybe this should be a ::new
method intead? I'm not really sure when new is preferred.
src/types/objects.rs
Outdated
|
||
impl From<Vec<ObjectCandidate>> for AllObjectCandidates { | ||
fn from(mut source: Vec<ObjectCandidate>) -> Self { | ||
source.sort_by_key(|candidate| (candidate.source.clone(), candidate.location.clone())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs say:
For expensive key functions (e.g. functions that are not simple property accesses or basic operations), sort_by_cached_key is likely to be significantly faster, as it does not recompute element keys.
Since at least location.clone()
allocates a new string, you should try to optimize this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, didn't spot the cached version.
This cloning is annoying, I didn't know what to do better about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, why can’t you just use (&candidate.source, &candidate.location)
? oh yes, because sort is mut
, or does it basically take care of that internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's the tuple that makes this impossible.
match self | ||
.0 | ||
.binary_search_by_key(&(source, location), |candidate| { | ||
(candidate.source.clone(), candidate.location.clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the same applies here. Maybe factoring out the sorting function would be a good idea, as that would also format this a lot nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what your suggestion is here. There is no cached variant, which is probably because this never needs to compute the same key twice. As this is currently only in the From
and here I don't think it's worth further factoring out. Once this grows more methods, like .set_unwind()
that'll probably change though at which point I think it makes more sense to do that.
just curious, since you have so many snapshot changes: is the sorting of the candidates stable? should it be? or just for the tests at least? |
fa3b54a
to
1585d05
Compare
Since this commit it explicitly is, since it now sorts on (sourceid, location). So this is hopefully the last time this order is completely changed in all the snapshot changes. |
ac6d6b6
to
ff18f8c
Compare
This updates the symcache to also add the debug status to the DIF candidates list returned in the modules list. It introduces a newtype to handle all the candidates and re-arranges where impls of types live a little.
ff18f8c
to
b385b43
Compare
So this went through a bit of a git hell after the target branch got merged into master. The diff is now correct again and is reviewable. Please do so. 👼 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to find a solution to the excessive cloning, but I think this is fine for a first pass
No description provided.