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
Implement searchable
property for marker schema
#4352
Conversation
Codecov ReportBase: 88.37% // Head: 88.36% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4352 +/- ##
==========================================
- Coverage 88.37% 88.36% -0.01%
==========================================
Files 282 282
Lines 25314 25351 +37
Branches 6826 6828 +2
==========================================
+ Hits 22371 22402 +31
- Misses 2731 2737 +6
Partials 212 212
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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 looks pretty good to me!
I don't see an upgrader part for these markers: https://searchfox.org/mozilla-central/rev/404408660a4d976e2ac25881cb1e1f2712f2d430/tools/profiler/core/MicroGeckoProfiler.cpp#47-88
do you think we could add one?
const searchableFields = schema.data.filter((field) => | ||
searchableFieldKeys.includes(field.key) | ||
); | ||
for (const field of searchableFields) { |
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.
optional nit:
Doing this in 2 steps look a bit more complicated than how it could be. What about looping on schema.data
directly?
for (const field of schema.data) {
if (searchableFieldKeys.includes(field.key)) {
field.searchable = true;
}
}
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.
Same comment for the other versioning file
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.
Right, changed it, thanks :)
// 'target' wasn't included in our code before. But I thought this | ||
// would be a useful addition. |
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.
good idea! maybe target
wasn't existing at the time we implemented the custom search. Not sure :-)
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.
Yeah, it looks like this was added after the initial implementation.
case 'FileIO': { | ||
searchableFieldKeys = [ | ||
'filename', | ||
'operation', | ||
'source', | ||
'threadId', | ||
]; |
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.
Is this change necessary? I see it was already done in the gecko code.
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.
It's probably not. I wasn't sure if they were added initially when we implemented the markers 2.0 or added later but it looks like they are actually added while we are implementing schemas. So we don't need to change them except threadId. We don't have the threadId in the schema, so added it manually.
markerSchema: MarkerSchema, | ||
marker: Marker, | ||
searchRegExp: RegExp | ||
): boolean { |
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 wonder if we should set searchRegExp.lastIndex = 0
here too. Currently we should be good, but I'm concerned about how we might use this function in the future too.
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.
Yeah, that makes sense. Added it.
36961a3
to
35c1c5c
Compare
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.
Thanks for the review! I addressed your comments. Added the last commit, would you mind taking a quick look on it?
I don't see an upgrader part for these markers: https://searchfox.org/mozilla-central/rev/404408660a4d976e2ac25881cb1e1f2712f2d430/tools/profiler/core/MicroGeckoProfiler.cpp#47-88
do you think we could add one?
Good idea, added it.
const searchableFields = schema.data.filter((field) => | ||
searchableFieldKeys.includes(field.key) | ||
); | ||
for (const field of searchableFields) { |
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.
Right, changed it, thanks :)
// 'target' wasn't included in our code before. But I thought this | ||
// would be a useful addition. |
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.
Yeah, it looks like this was added after the initial implementation.
case 'FileIO': { | ||
searchableFieldKeys = [ | ||
'filename', | ||
'operation', | ||
'source', | ||
'threadId', | ||
]; |
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.
It's probably not. I wasn't sure if they were added initially when we implemented the markers 2.0 or added later but it looks like they are actually added while we are implementing schemas. So we don't need to change them except threadId. We don't have the threadId in the schema, so added it manually.
markerSchema: MarkerSchema, | ||
marker: Marker, | ||
searchRegExp: RegExp | ||
): boolean { |
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.
Yeah, that makes sense. Added it.
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.
looks good to me, thanks for checking!
35c1c5c
to
8de1e7b
Compare
Fixes #2780.
We also need to land the Bugzilla counterpart here. It Adds
searchable: true
to:name
andcategory
.This PR removes the manual filtering that we had and replaces it with checking all the payload's
searchable
fields.We also had to add an upgrader for both gecko and processed format and add
searchable
fields for these fields, so even though we remove these manual checks, search on older profiles work as expected.I wasn't sure if the performance would be the same but after doing some micro benchmarks, I've found out that it's at lest the same if not better. Here's a before and after running times of that getSearchFilteredMarkerIndexes that I measured.
Deploy preview:
I also got this profile from out spreadsheet that says "Awful lots of markers" :)
Before / After