Recent

Author Topic: [SOLVED] 2.2 win x64: ugly issue with virtual listview "Selected" performance  (Read 13736 times)

d7_2_laz

  • Hero Member
  • *****
  • Posts: 511
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #15 on: January 20, 2022, 09:58:14 pm »
Hi GetMen, i understand you patch file so that. within TCustomListView.GetSelection,
it adds the condiktion:
      if (not FOwnerData) or (FOwnerData and (FSelectedIdx > 0)) then
before the line:
      for i := 0 to Items.Count - 1 do

So basically it does the same as my version, but more elegant.
I did try your version modifying the  2.2  Win x64 customlistview.inc on my PC accordingly, rebuilt all and retested
- the testcases written for this special issue
- the whole app
and encountered no problems at all so far.  The app did behave (regarding this special issue #39324) just as expected and in the same way as before when having my own patch applied.
So for me your proposal is perfect!   :)

Unforrtunately i
- cannot speak about other OSses
- cannot say anything about this runtestsgui.lpi testing scenario,as  i don't know nothing about it.
Lazarus 3.2  FPC 3.2.2 Win10 64bit

balazsszekely

  • Guest
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #16 on: January 20, 2022, 10:07:30 pm »
@d7_2_laz
In you patch you set Result and FSelected nil, then immediately exit the function. After the for cycle there is one more line that is not executed. I will look at the testcase tomorrow.

d7_2_laz

  • Hero Member
  • *****
  • Posts: 511
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #17 on: January 20, 2022, 10:31:01 pm »
@GetMem, yes i see. You as well as me set (if the condition is met) both FSelected and Result to nil (that is both necessary), but you additionally pass the inclusion of  lffSelectedValid into FFlags.

Although i noticed no side-effects so far. apparently it needs to be decided if this setting of the flag is necessary resp. wanted or not, regarding the special condition.
This had not been in my focus so far.  At least i did not exclude it by intention.
Lazarus 3.2  FPC 3.2.2 Win10 64bit

balazsszekely

  • Guest
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #18 on: January 20, 2022, 10:32:04 pm »
I ran the test, there is one failure in the popup menu section, not related to our changes.

d7_2_laz

  • Hero Member
  • *****
  • Posts: 511
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #19 on: January 20, 2022, 11:48:11 pm »
There are two usage contexts for the flag lffSelectedValid in speech.

-the one is the current TCustomListView.GetSelection itself,
  where apparently it's not critical if the same "if" (with the same result "nil") might be entered twice; it's not costly though.

-  the other is TCustomListView.CNNotify around line 346
  where your version would prohibit (as having lffSelectedValid included in FFlags) and my version would allow to enter this if:
 
          // select
          if (((nm^.uOldState and LVIS_SELECTED) <> (nm^.uNewState and LVIS_SELECTED)))
          or (not (lffSelectedValid in FFlags) and (nm^.uNewState and LVIS_SELECTED <> 0)) then
          begin
            // select state changed

  Theoretically .... but for the condition that Selected has detected to be nil, the latter "if" will even not be reached though.
  At least a breakpoint here has no effect for me, it does not stop here.

So i would guess: it does not matter if  -for the case of Selected is nil -  the flag is included or not. At öeast i saw no difference in practice (also when playing aroung with keyboard/mouse with/without Shift and or Ctrl, KeyDown/KeyUp), no impact.

Maybe there is some additional specific reaon to - for the case Selection had been found to be nil - include lffSelectedValid in FFlags or not?  My vote so far would be: Go for your change ...
Lazarus 3.2  FPC 3.2.2 Win10 64bit

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9794
  • Debugger - SynEdit - and more
    • wiki
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #20 on: January 21, 2022, 03:01:32 pm »
Code: Pascal  [Select][+][-]
  1.       if OwnerData And (FSelectedIdx=-1) then begin
  2. ...
  3.          Exit;
  4.       end;
  5. ...
  6.       for i := 0 to Items.Count - 1 do

This still allows for virtual views (aka "OwnerData") to enter the loop in
Code: Pascal  [Select][+][-]
  1. function TCustomListView.GetSelection: TListItem;
  2. ...
  3.   if OwnerData and not MultiSelect then
  4. ...
  5.   else begin
  6. ...
  7.     if not (lffSelectedValid in FFlags) or MultiSelect then begin
  8.       FSelected := nil;
  9.       for i := 0 to Items.Count - 1 do
  10.  

Which IMHO should not happen. (At least not in as many situations, as the suggested patch allows for).

The patch could be part of the solution. But IMHO it can be improved.


The suggested patch "if OwnerData And (FSelectedIdx=-1)" prevents entry of the loop, if it is known that there is no selection at all.
But, if the last 2 items (out of 50.000 items) are selected, the loop is entered. And (almost) all items are iterated.
Btw, if indeed "FSelectedIdx=-1" always and reliable indicates that there is no selection at all, then this could be added for non-virtual views too (so long as no better solution exists for either).

And even, if with the suggestion to only loop internally, this is still a lot of items to loop.
(See https://gitlab.com/freepascal.org/lazarus/lazarus/-/issues/39324#note_811612058 => check "lisSelected in FStates", no need to call "OnData")


TCustomListView.CNNotify should whenever possible keep track of the "first selected item.

If, it can not do that (probably cases where the current first-selected, is un-selected via ctrl-click, it could still store that index as "start point" for the search (but that is optional).

function TCustomListView.GetSelection: TListItem;
Should at the very least cache the found index.  (CNNotify can unset the cache)

This still leaves cases, where all items need to be searched. But it should reduce the odds as much as possible.




Code: Pascal  [Select][+][-]
  1. procedure TCustomListView.SetSelection(const AValue: TListItem);
has another loop, that for would call "OnData" for all items, despite only needing "lisSelected in FStates".



Btw: for OwnerData "ListView.ItemFocused" is not updated.

d7_2_laz

  • Hero Member
  • *****
  • Posts: 511
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #21 on: January 21, 2022, 04:26:22 pm »
Martin_fr, i think you are right. Tried to verify the scenario with my testcase (as of reply #8) as well as with myp and this happens:

With the patch (as from GetMems reply #9) the early population phase of the programs no is fast again.
Before, without the patch, runtime might explode due to some "if Selected = nil" somewhere within  routins).
This is now solved by the patch-

But when navigating to the last element in the list and selecting it, then "if Selected = nil" lets the runtime, via calls of OnData, explode again.

In my app the use case is as follows: folder treeview, tabbed listview(s): select "C:\Windows\WinSxS".
Without the patch: listview's population takes forever. With the patch: runs instantly again.

Select item #5 within the list: processing is instantly
Select last item within the list: runtime takes forever again (when "if Selected .." comes into play)

So the patch solves parts of the problem, but obviously is not sufficient for itself.

I retried with versions of the app generated with 2.0.12 (and 2.ß.10 and Delphi 7):
population: instantly
selecting last item: processing is instantly
Good times, sigh ....
Lazarus 3.2  FPC 3.2.2 Win10 64bit

balazsszekely

  • Guest
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #22 on: January 21, 2022, 04:55:13 pm »
@Martin_fr, @d7_2_laz
Another solution is to revert d6c215a4("make ListView.Selected return the first selected item also when OwnerData=True). Everything will be fast again.
I don't think we should care if ListView.Selected points to the first or last selection. If somebody wants to know the selected items when multiselect is true, it has to loop through the items anyway.

 

d7_2_laz

  • Hero Member
  • *****
  • Posts: 511
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #23 on: January 21, 2022, 05:14:51 pm »
That's exactly what i did before (on application level).
So to say, it's a loop just by demand, just when needed. But not affecting several ops continuously.

One needs to keep in mind that this might be a loop around all items though
(as with keystroke  Ctrl and Click, you can select interrupted sequences, eg. item #1, item #10, item #99)
Lazarus 3.2  FPC 3.2.2 Win10 64bit

d7_2_laz

  • Hero Member
  • *****
  • Posts: 511
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #24 on: January 22, 2022, 11:38:27 am »
A. Current status;  just more realistic test case

For to have a more real-life impression showing the impact of the remaining part of the issue that is not yet covered by the patch, i created a test case using a ShellViewDemo i picked up from an older discussion in this forum. (Attached)
My motiviation to post this is to show that it is not a purely theoretical thing.

- Select a larger folder in the treeview, eg.  C:\Windows\System32  or  (hard-core:) C:\Windows\WinSxS
- In the listview, press <Ctrl><End> for to go to the end of the list
- Click the last row ... and wait ... wait ... until a message box appears that tells which item had been selected
More than 6 seconds for "System32", 30 seconds for "WinSxS"   ...

That's exactly the situation i'm having in my own app.
New behaviour since 2.2. RC1.

B. Solution compromise

The following modification of TCustomListView.GetSelection would (equivalently to reverting the RC1 patch which caused the issue) regain the full speed of 2.0.12 with the price, of course, to return again a last element instead of the first element.

Code: Pascal  [Select][+][-]
  1.     if not (lffSelectedValid in FFlags) or MultiSelect then
  2.     begin
  3.       FSelected := nil;
  4.  
  5.       // d7_2_laz: Patch https://gitlab.com/freepascal.org/lazarus/lazarus/-/issues/39324
  6.       if (not FOwnerData) or (FOwnerData and (FSelectedIdx > 0)) then begin
  7.          if (MultiSelect And (FOwnerData and (FSelectedIdx > 0))) then
  8.              FSelected := Items[FSelectedIdx]
  9.          else
  10.          for i := 0 to Items.Count - 1 do begin
  11.               if Items[i].Selected then begin
  12.                  FSelected := Items[i];
  13.                  break;
  14.               end;
  15.          end;
  16.       end;
  17.  
  18.       Include(FFlags, lffSelectedValid);
  19.     end;
  20.  

Try the change using the test case ....
Both are attached here.
Plus:  full performance is back again ! The overall navigation performance of the app is noticeably better (and better than with 2.0.12)!
Minus: Last instead of first selected element will be returned at multiselect
Personally for me, the Plus would be much much more important than the Minus. First/last might be handled on application level.
 
Are there any different opinions?

What about - as compromise - to imply a property (maybe defaulted to "return first item", Delphi-like) that allows the developer to choose which behaviour / option he prefers?
Something like FOwnerDataMultiSelectReturnLast := True (defaulted to False) .. FMultiSelectDelphiCompatility := true or such, let it up to the user.
if OwnerDataMultiSelectDelphiCompatility then .. existing code .. else ... reverted code as aboce.
I'd definitely (definitely!) would choose "speed" in very basic, fundamental operations and invest a bit of work on application level for the few cases when "first" is needed instead of "last".

What do you think? I'd highly be interested in!

Could this be a way out to shorten up this endless story, where i'm triggering the same issue again and again, because i'm suffering on it?

Lazarus 3.2  FPC 3.2.2 Win10 64bit

ASerge

  • Hero Member
  • *****
  • Posts: 2223
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #25 on: January 22, 2022, 03:03:33 pm »
Micro remark: the following two expressions are equal:
if (not FOwnerData) or (FOwnerData and (FSelectedIdx > 0)) then
if (not FOwnerData) or (FSelectedIdx > 0) then

d7_2_laz

  • Hero Member
  • *****
  • Posts: 511
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #26 on: January 23, 2022, 01:04:53 pm »
Yep ASerge, right .. :
Additionally, the "FSelectedIdx > 0" better should read ">= 0", otherwise a first item won't be fetched..
Lazarus 3.2  FPC 3.2.2 Win10 64bit

balazsszekely

  • Guest
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #27 on: January 23, 2022, 01:34:03 pm »
@d7_2_laz
Please add the latest info to the bugtracker, otherwise it will be lost.  I see the issue is now assigned, maybe a fix will be available soon in trunk.

d7_2_laz

  • Hero Member
  • *****
  • Posts: 511
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #28 on: January 23, 2022, 03:15:11 pm »
GetMem, is there even an interest to have such kind of workaround? No substantially feedback Yes/No in this respect so far.
Everybody would like to have a proper solution, where the problem does not arise in the first place. Me too.
The more because on application level there will be still consecutive struggles with Item loops.
Is such a workaround (which is not a comprehensive solution, as Martin pointed out) in the path of decision?
I don't know. Meanwhile i'm loosing energy to trigger this topic again and again and close to give up.
Lazarus 3.2  FPC 3.2.2 Win10 64bit

balazsszekely

  • Guest
Re: 2.2 win x64: two persistent ugly issues with virtual listview "Selected"
« Reply #29 on: January 23, 2022, 03:34:11 pm »
GetMem, is there even an interest to have such kind of workaround? No substantially feedback Yes/No in this respect so far.
Everybody would like to have a proper solution, where the problem does not arise in the first place. Me too.
The more because on application level there will be still consecutive struggles with Item loops.
Is such a workaround (which is not a comprehensive solution, as Martin pointed out) in the path of decision?
I don't know. Meanwhile i'm loosing energy to trigger this topic again and again and close to give up.
I don't know. Personally I would revert d6c215a4, this would solve the speed issue, then I would try to address the "last instead of first selected element on multiselect" separately. In my opinion the later issue is not critical, it's just a mild inconvenience at best. However fixing it is no walk in the park and most likely will introduce new issues.
Again the bug is not assigned to me, so I don't want to insist on going in one way or the other.

 

TinyPortal © 2005-2018