-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Web Inspector: Timelines: Unable to load more instances of a type in JavaScript Allocations view #32721
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
Conversation
|
EWS run on previous version of this PR (hash ba5dded) |
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 the siblingNode for-loop is simple enough that we don't need the lambda?
dataGridMatchNodeAgainstCustomFilters(node)
{
console.assert(node);
if (!(node instanceof WI.HeapSnapshotInstanceFetchMoreDataGridNode))
return true;
for (let siblingNode of node.parent.children) {
if (!siblingNode.hidden && siblingNode !== node)
return true;
}
return false;
}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 I prefer the explicitly named function call.
The WI.HeapSnapshotInstanceFetchMoreDataGridNode entry should be hidden only when there are no visible siblings, regardless of conditions.
Consider these cases:
- no filtering, many nodes
- filtering, no matching nodes
- filtering, matching many nodes
Reading node instanceof WI.HeapSnapshotInstanceFetchMoreDataGridNode && !hasVisibleSiblingNodes() is clearer about that intent.
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.
IMO the inlined version is nicer to read as it keeps the logic more localized (i.e. we only really care about this code in the case that we're dealing with a WI.HeapSnapshotInstanceFetchMoreDataGridNode so "polluting" the outside scope is somewhat not ideal)
but putting that aside for a moment
i don't think this is necessarily the correct fix
in other places that do things like dataGridMatchNodeAgainstCustomFilters, usually the first check is to even see if there is an active filter, which i think in this case can be checked via this._dataGrid.filterText
as far as still showing it when filtering, i think we may just want to successively iterate over .previousSibling instead of grabbing the .parent.children since we know that it will always be the last child (and ideally we'd make this a member function of WI.HeapSnapshotInstanceFetchMoreDataGridNode since that's really where this logic should live)
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.
in other places that do things like dataGridMatchNodeAgainstCustomFilters, usually the first check is to even see if there is an active filter, which i think in this case can be checked via this._dataGrid.filterText
The existence or absence of a filter text is not relevant to showing a WI.HeapSnapshotInstanceFetchMoreDataGridNode entry.
Consider the case where even with a filter there are still more results than the batch limit. (Filtering does not operate on the entire list of results, it operates only on created nodes).
The only thing that is truly relevant is whether there are more than WI.HeapSnapshotClassDataGridNode.ChildrenBatchLimit sibling nodes visible. But we don't have to be check that precisely.
The filter text matches only against the entries for the column names of the WI.DataGrid:
(className is the relevant column name for WI.HeapSnapshotInstanceDataGridNode and WI.HeapSnapshotClassDataGridNode)
| if (filterableData.some((value) => filterRegex.test(value))) { |
WebKit/Source/WebInspectorUI/UserInterface/Views/DataGridNode.js
Lines 169 to 194 in 8f07bf9
| get filterableData() | |
| { | |
| if (this._cachedFilterableData) | |
| return this._cachedFilterableData; | |
| this._cachedFilterableData = []; | |
| for (let column of this.dataGrid.columns.values()) { | |
| if (column.hidden) | |
| continue; | |
| let value = this.filterableDataForColumn(column.columnIdentifier); | |
| if (!value) | |
| continue; | |
| if (!(value instanceof Array)) | |
| value = [value]; | |
| if (!value.length) | |
| continue; | |
| this._cachedFilterableData.pushAll(value); | |
| } | |
| return this._cachedFilterableData; | |
| } |
An instance type (className) that has more results than the limit will still have more results than the limit even when matched by the filter text.
The filter text isn't currently matched against more granular data within each instance.
This means we can return early for any visible sibling nodes instead of having to count them.
Screen.Recording.2024-09-02.at.13.33.45.mov
as far as still showing it when filtering, i think we may just want to successively iterate over .previousSibling instead of grabbing the .parent.children since we know that it will always be the last child (and ideally we'd make this a member function of WI.HeapSnapshotInstanceFetchMoreDataGridNode since that's really where this logic should live)
This is a good idea. 👍
I updated the patch to have WI.HeapSnapshotInstanceFetchMoreDataGridNode.hasVisibleSiblingNodes(). It satisfies both the desires to see the check inline ...
if (node instanceof WI.HeapSnapshotInstanceFetchMoreDataGridNode && !node.hasVisibleSiblingNodes())
... and the desire to explicitly name the method for clarity.
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 existence or absence of a filter text is not relevant to showing a
WI.HeapSnapshotInstanceFetchMoreDataGridNodeentry.
it absolutely is. if there's no filter text then there's no reason to filter out WI.HeapSnapshotInstanceFetchMoreDataGridNode regardless of its or sibling state. the entire point of dataGridMatchNodeAgainstCustomFilters is to support filtering beyond just text (e.g. a button that has some special functionality). having something like this at the beginning
if (!this._dataGrid.filterText)
return true;will be a much quicker short-circuit than having to do an instanceof check and look at siblings
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 entire point of dataGridMatchNodeAgainstCustomFilters is to support filtering beyond just text (e.g. a button that has some special functionality). having something like this at the beginning
If that was truly the point and dataGridMatchNodeAgainstCustomFilters was an additive filter, it should not be called if there is no filter text. But that's not what I'm getting from your suggestion or the implementation:
WebKit/Source/WebInspectorUI/UserInterface/Views/DataGrid.js
Lines 487 to 498 in ef7ef86
| function matchTextFilter() | |
| { | |
| if (!filterableData.length || !filterRegex) | |
| return true; | |
| if (filterableData.some((value) => filterRegex.test(value))) { | |
| flags.expandNode = true; | |
| return true; | |
| } | |
| return false; | |
| } |
WI.DataGrid._applyFiltersToNode(node) is intentionally designed to matchTextFilter() if there is no filter text which is then used in conjunction with
| if (matchTextFilter() && this.matchNodeAgainstCustomFilters(node)) { |
Seems odd to have to recheck filter text in the delegate function if the point of the dataGridMatchNodeAgainstCustomFilters is to further filter results.
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 not additive, it's alternative (hence the "custom" name)
the idea is that it can be used to do other kinds of filtering beyond just matching text
for example, the Sources Tab has a button next to the text filter that will cause only resources with issues to be shown. it doesn't matter whether the developer has entered anything into the text filter in order for that button to have an effect. so if the Sources Tab used a WI.DataGrid then it would need functionality like dataGridMatchNodeAgainstCustomFilters in order to have that effect
another example is the resource type WI.ScopeBar in the Network Tab. back when that used a WI.DataGrid (instead of the WI.Table it currently uses) it likely would've used dataGridMatchNodeAgainstCustomFilters to implement that kind of custom filtering (again regardless of whether the developer had input anything into the URL filter)
in fact, dataGridMatchNodeAgainstCustomFilters being called regardless of whether there's a text filter is a critical part of how WI.TimelineView works as without that we'd need yet another mechanism for ensuring that only timeline entries within the selected time range are shown
in this case, you're using dataGridMatchNodeAgainstCustomFilters to do an additional filter step beyond just the text filter (i.e. there's no button or WI.ScopeBar that has filtering functionality completely independent of the text filter), so we should check that the developer is doing a text filter before bothering to do anything else
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.
also ive been thinking about this a bit more, and i think there needs to be a FIXME here
ideally, we'd only ever show WI.HeapSnapshotInstanceFetchMoreDataGridNode if we know for a fact that something hidden matches the (text) filter, as otherwise we could be misleading the developer into thinking that when it's not true
similarly, if we're only showing WI.HeapSnapshotInstanceFetchMoreDataGridNode if one of its previous siblings matches the (text) filter, then in situations where none do we would not show the WI.HeapSnapshotInstanceFetchMoreDataGridNode even though something hidden by it might match the (text) filter
as such, what we really need is a way to search the entire heap snapshot for matches so that we can gather all of the data to show instead of using WI.DataGrid logic to just search what's in the UI
i think this should probably be a followup though, as that's likely going to be much more complicated, and in the meantime we should fix the bug of allowing WI.HeapSnapshotInstanceFetchMoreDataGridNode to be shown when not filtering for anything
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 disagree with you, but I added the check for the filter text anyway since it won't affect the behavior of this patch at all.
The examples you provided are indeed about filtering results. They're orthogonal to the issue here, where filtering is used as a hack IMO to toggle the visibility of a UI control WI.HeapSnapshotInstanceFetchMoreDataGridNode that happens be mixed in with the data nodes.
If filtering would work against the full dataset and granularly, and I agree that's a better experience, that would mean toggling the visibility of the WI.HeapSnapshotInstanceFetchMoreDataGridNode based on content. Whether or not a filter text is present would still be irrelevant; the number of filtered results matters.
it's not additive, it's alternative (hence the "custom" name)
the idea is that it can be used to do other kinds of filtering beyond just matching text
If that's true, then this becomes that:
- if (matchTextFilter() && this.matchNodeAgainstCustomFilters(node))
+ if (matchTextFilter() || this.matchNodeAgainstCustomFilters(node))But since we're pretending that results with no filterable data did match, we do this check in the delegate:
if (!this._dataGrid.filterText)
return true;
Either way, this patch serves its purpose with and without this addition so I'm putting it in.
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 examples you provided are indeed about filtering results. They're orthogonal to the issue here, where filtering is used as a hack IMO to toggle the visibility of a UI control
WI.HeapSnapshotInstanceFetchMoreDataGridNodethat happens be mixed in with the data nodes.
this bug was caused by 248198@main which specifically added support for filtering, so im not really sure how this is not related to filtering
also btw WI.HeapSnapshotInstanceFetchMoreDataGridNode is only added if there are more than 100 values, so it only exists in those cases (i.e. it's not like it's always added and selectively hidden based on whether or not there are more things to show)
the ideal behavior is that if no filter text is provided, these would not be filtered out at all, so the only situation in which they should be hidden is if the filter text doesn't match
If filtering would work against the full dataset and granularly, and I agree that's a better experience, that would mean toggling the visibility of the
WI.HeapSnapshotInstanceFetchMoreDataGridNodebased on content. Whether or not a filter text is present would still be irrelevant; the number of filtered results matters.
the filter text would be what determines wither the WI.HeapSnapshotInstanceFetchMoreDataGridNode is shown. if there's no filter text then it should always be shown. if there is filter text then it would only be shown if something hidden by it matches (or in the case of this PR, something else before it matches).
it's not additive, it's alternative (hence the "custom" name)
the idea is that it can be used to do other kinds of filtering beyond just matching textIf that's true, then this becomes that:
- if (matchTextFilter() && this.matchNodeAgainstCustomFilters(node)) + if (matchTextFilter() || this.matchNodeAgainstCustomFilters(node))But since we're pretending that results with no filterable data did match, we do this check in the delegate:
if (!this._dataGrid.filterText) return true;
i agree that the existing code isn't written super well, but it's effectively doing what you describe because matchTextFilter() will always be true if there is no filter text (because this._textFilterRegex will not be set)
ba5dded to
0e282c0
Compare
|
EWS run on previous version of this PR (hash 0e282c0) |
0e282c0 to
c94eaf4
Compare
|
EWS run on current version of this PR (hash c94eaf4) |
…JavaScript Allocations view https://bugs.webkit.org/show_bug.cgi?id=278667 rdar://133810318 Reviewed by BJ Burg. For heap snapshot trees with many entries, instances are rendered batched in groups of 100. A `WI.HeapSnapshotInstanceFetchMoreDataGridNode` with controls to generate the DOM for the next batch or the full list is created, but it marked as `hidden` as a side-effect of https://bugs.webkit.org/show_bug.cgi?id=157582. A `WI.DataGrid` is virtualized to only display the limited number of data grid nodes that are visible in the viewport. Any `WI.DataGridNode` marked as `hidden` is skipped in `WI.DataGrid.updateVisibleRows()`: ``` let revealedRows = this._rows.filter((row) => row.revealed && !row.hidden); ``` `WI.HeapSnapshotInstanceFetchMoreDataGridNode` gets marked as hidden when inserted inserted: `WI.DataGrid.insertChild(node)` -> `WI.DataGrid._applyFiltersToNodeAndDispatchEvent(node)` -> `WI.DataGrid._applyFiltersToNode(node)` where a delegate filtering method is called: `WI.HeapSnapshotContentView.dataGridMatchNodeAgainstCustomFilters()`. This delegate filter explicitly skips `WI.HeapSnapshotInstanceFetchMoreDataGridNode` nodes: ``` dataGridMatchNodeAgainstCustomFilters(node) { console.assert(node); if (node instanceof WI.HeapSnapshotInstanceFetchMoreDataGridNode) return false; return true; } ``` Combined with the virtualization logic from `WI.DataGrid.updateVisibleRows()` this results in always hiding the controls to load more entries. The intent was likely to skip showing these controls when there are no matches for a filter query. This patch ensures that the delegate filter checks for visible sibling data grid nodes when deciding whether to show the controls to load more entries. The filter query matches against the top-level instance type, so the number of matched individual instances of that type will not vary. Therefore, the controls to load more entries don't have to update their counts. * Source/WebInspectorUI/UserInterface/Views/HeapSnapshotContentView.js: (WI.HeapSnapshotContentView.prototype.dataGridMatchNodeAgainstCustomFilters.hasVisibleSiblingNodes): (WI.HeapSnapshotContentView.prototype.dataGridMatchNodeAgainstCustomFilters): Canonical link: https://commits.webkit.org/283215@main
c94eaf4 to
41ae460
Compare
|
Committed 283215@main (41ae460): https://commits.webkit.org/283215@main Reviewed commits have been landed. Closing PR #32721 and removing active labels. |
41ae460
c94eaf4