search

StanfordLegion / prof-viewer

Legion Prof Viewer
Apache License 2.0
0 stars 5 forks source link

Search returning out-of-bounds results #30

Open bryevdv opened 1 year ago

bryevdv commented 1 year ago

When the viewport is zoomed into a smaller interval, and the search is invoked, then the returned results can include items outside the viewport, contrary to expectation. The root cause appears to be a missed cache invalidation for unexpanded results.

cc @lightsighter for a profile and search term to use to reproduce.

lightsighter commented 1 year ago

I've sent the data and the instructions offline since they are too big to upload here.

elliottslaughter commented 1 year ago

Just wanted to give one point of direction to @bryevdv. What I showed you on the call was the draw path. But I think invalidating the meta tiles is not the right approach there. I think the place to check is inflate_meta, which is called by the search path (and already handles all the logic for if we're searching over unexpanded processors). Basically, this code needs invalidation logic:

https://github.com/StanfordLegion/prof-viewer/blob/2f8dfcd2065ac6c0ea072adab5bf397d7c553fdf/src/app.rs#L783-L787

(If you follow up the call stack you'll get to the place where this is called in the search algorithm.)

elliottslaughter commented 12 months ago

I think we fixed this with #32?

bryevdv commented 11 months ago

@elliottslaughter I don't think so? #32 just added the whole-word search option, which makes it easier to pare down search for certain kinds of results, e.g. ids that have fences like <123>

elliottslaughter commented 11 months ago

You're right. The cache invalidation bug is still there. Sorry about the noise.

bryevdv commented 11 months ago

Hi @elliottslaughter @lightsighter I want to make sure I understand this bug.

By contrast: If I start from the zoomed out view with a search result (bullet two above), and then zoom away, without resetting anything first, then the result stays in the search, which I would not expect:

Screenshot 2023-11-13 at 13 55 33

Is this different from the original reported bug? Regardless, just to confirm: the search results should reset when the view interval changes?

lightsighter commented 11 months ago

Reset the search, zoom into a region without the "71" then search again — no result shown, still all good?

If I scroll right or left, will the results automatically refresh?

I had thought this was the reported error case, and expected to see a result show up even when searching in the zoomed-away area. But it seems to be working as I'd expect.

I honestly don't remember what I was trying to do that was behaving funny. I just remembered you guys agreed with me that it was strange when I showed it to you. 😇

By contrast: If I start from the zoomed out view with a search result (bullet two above), and then zoom away, without resetting anything first, then the result stays in the search, which I would not expect:

By "zoom away" do you mean "zoom in"? If so I think this might have been my case too. I was trying to refine a search and wanted to see results getting pruned out as the window size shrunk.

Regardless, just to confirm: the search results should reset when the view interval changes?

I think in general this in the invariant we want to maintain: the search results should always reflect what is visible. Anything that causes the viewport to change, should cause the search results to refresh.

elliottslaughter commented 11 months ago

I think the bug involved a combination of zooming + the "search unexpanded processors" option. Because those processors are unexpanded, the cache invalidation logic doesn't kick in and we get stale results. I think that's why I made my comment about inflate_meta here: https://github.com/StanfordLegion/prof-viewer/issues/30#issuecomment-1758090133

Try this in a fresh profile:

  1. Find the task you want to search for
  2. Collapse all the processors (so you can't see it anymore)
  3. Search (should be empty; this is correct)
  4. Click "search unexpanded processors"
  5. It should appear in search now (this is correct)
  6. Zoom away (anywhere that the task isn't, so the task isn't in the view interval)
  7. The task should still appear in the search (this is the bug)