1

I know there have been I would say similar questions, but none of them fits my case -> see my already tried attempts below.

So what I'm trying to do is make a advanced searchBar. Therefore, everytime the searchParam changed, I need to execute code to check whether my TableViewItems (Array) match the criteria -> filter.

When smo. for example types 3 characters, my code checks this string. But this will take a while to get all results.

The Problem: When smo. then types the 4th character, I want to stop the previous execution and start a new one with the string of 4.


My Attempts:

  • Using DispatchWorkItem:

    The problem here is that it only changes a Boolean and it take 10sec+ for the code to recognize that it changed. Works if I execute it .sync instead of .async, but it freezes the app for more than 10sec

  • Using DispatchQueue:

    Can't be stopped, only paused -> so will remain in memory -> will spam memory

  • Checking Boolean in every for loop:

    Same as with DispatchWorkItem, will take 10+ sec to recognize a change


Current Code: (not very important)

func searchBar(_ searchBar: UISearchBar, textDidChange searchText: String) {
        self.stop = true
        if people != nil {

            if searchText.isEmpty {
                searchedPeople = people
                self.stop = false
            } else {
                self.searchedPeople = []
                self.tableView.reloadData()
                workItem = DispatchWorkItem(qos: .userInitiated, flags: []) {
                    outter: for i in 0...searchText.count - 1 {
                        if self.stop { self.stop = false;break outter } //
                        if i == 0 {
                            inner: for person in self.people! {
                                if self.stop { self.stop = false;break outter } //
                                let name = (person.Vorname != nil ? person.Vorname! + " " : "") + (person.Nachname ?? "")
                                if name.lowercased().contains(searchText.lowercased()) {
                                    print("Found: \(name), \(searchText), Exact Match")
                                    self.searchedPeople?.append(person);DispatchQueue.main.async { self.tableView.reloadData()}; continue inner
                                }
                            }
                        } else {
                            let firstP = searchText.prefix(i+1)
                            let lastP = searchText.suffix(searchText.count - i + 1)

                            inner2: for person in self.people! {
                                if self.stop { self.stop = false;break outter } //
                                let name = (person.Vorname != nil ? person.Vorname! + " " : "") + (person.Nachname ?? "")
                                if name.lowercased().contains(firstP.lowercased()) && (name.lowercased().contains(lastP.lowercased())) {
                                    print("Found: \(name), \(searchText), First: \(firstP), Last: \(lastP)")
                                    self.searchedPeople?.append(person)
                                    DispatchQueue.main.async { self.tableView.reloadData()}
                                    continue inner2
                                }
                            }
                        }
                    }
                }
                //execute
                DispatchQueue.async(execute: workItem)

            }
        }
    }
John Smith
  • 573
  • 6
  • 22
  • 1
    Why would it take 10 seconds to update a boolean? Something seems off there. Could you show us the code you've used to cancel your `DispatchWorkItem`? – Charles Srstka Jul 24 '18 at 19:23
  • This isn't really an answer to the question, but your code is searching for people in an inefficient way. CoreData and other tools can help you do this with an index that would make the search time trivially small; might be worth investing some time into setting a data system up if you're doing this sort of thing a lot. – sudo Jul 24 '18 at 19:24
  • @sudo I can look into that suggestion, but the search time is fine for me right now, even if multiple executions are at the same time, but the old exec. is still adding the results of the 3 chars to my tableview, even though i already typed 4 – John Smith Jul 24 '18 at 19:28
  • @CharlesSrstka I think the value itself chenges instantly but the code, where it checks whether true is already in memory I assume due to the `.async` execution. Bc if I run it in `.sync` mode it works fine, its just that sync take 10+ sec – John Smith Jul 24 '18 at 19:31
  • @JohnSmith If it's a `DispatchWorkItem`, why are you using `.sync` or `.async` to call its `cancel()` method on a queue? The `DispatchWorkItem`'s `isCancelled` boolean should already be protected internally by a mutex or semaphore (it's definitely that way with the Foundation equivalent, `NSOperation`), so you should just be able to call `.cancel()` and `isCancelled` without going through any additional hoops. – Charles Srstka Jul 24 '18 at 19:37
  • Don't compare lowercased strings. Use the compare function with the right options (case insensitive etc. ). Each comparison you do creates two auto-released strings. – gnasher729 Jul 24 '18 at 19:51
  • @CharlesSrstka I've played around with Dispatch a lot the past days and it doesn't matter how you call it (yes the one call might be unnecessary), the thing is that .cancel doesn't cancel the execution like I want it, it just sets the isCancelled boolean to true, nothing else – John Smith Jul 24 '18 at 20:20
  • @CharlesSrstka To your first comment: I did just run workItem.cancel() and in the code block, instead of checking if the Boolean `stop` is true, I did check if `.isCancelled` is true – John Smith Jul 24 '18 at 20:26
  • @JohnSmith Right, and then you check `.isCancelled` in your loops. As soon as the item's been cancelled, everything should stop immediately, with no ten second delays. – Charles Srstka Jul 24 '18 at 20:44
  • Please read my 4th comment above where I already described it. I already said there that That it takes 10+ sec before the execution really stops. Do I know 100% why? No, but I assume that it has something to do with .async – John Smith Jul 24 '18 at 20:53
  • If you put a log in your work code, can you verify that it's really the work code that's taking 10 seconds to finish, and not just the UI getting locked up? – Charles Srstka Jul 24 '18 at 20:55
  • Actually looking at your comment again, I'm fairly convinced that @sudo's explanation is correct; you're overwhelming the UI thread. The worker code takes 10 seconds to finish if you use `.sync()`, because then it has to wait for the UI before it can complete, and the UI is stuck rapid-fire reloading the table a crazy number of times. – Charles Srstka Jul 24 '18 at 20:57
  • @CharlesSrstka I'm adding it to the array which is the datasource of the table also. And 10+ sec it is still adding results to the array so it is clearly not the ui – John Smith Jul 24 '18 at 21:01
  • @JohnSmith But only if you use `.sync`, right? – Charles Srstka Jul 24 '18 at 21:02
  • 1
    The approach I normally take is to start a timer for say, 1 second, when the user types a character and restart it when they type the next character. If the timer fires then it means that they haven't typed anything for a second and then perform the search. Normally it isn't work cancelling a search operation because they should be pretty quick. How many names are you searching? – Paulw11 Jul 24 '18 at 21:08
  • ^ This is a good idea. – Charles Srstka Jul 24 '18 at 21:10
  • @Paulw11 probable around 6000. And I already converted them from a json to a custom "Person" Class which has 6 string variables (name is one of them) – John Smith Jul 24 '18 at 21:13
  • 1
    Then you should definitely look at Core Data or at least how you are performing the search. Your current code is very inefficient. I have an app that searches 24,000 names in Core Data and results are instantaneous. There is no need to consider timers or cancelling operations since the results are returned before the person has typed the second character. – Paulw11 Jul 24 '18 at 21:14
  • Also, don't reload the tableview each time you get a new match. Find all of the matches and then reload the tableview – Paulw11 Jul 24 '18 at 21:16
  • @Paulw11I tried that, same result – John Smith Jul 24 '18 at 21:19
  • ^ For reference: And in applications that do need live updating as results come in (doing what Spotlight does, for instance), you can create a `needsReload` boolean and simply set that to `true` when a result comes in. Then, you can have something on the main thread check every 0.1 seconds or something to see if the bool is true, and reload the table if it is. This way, if a thousand results all come in in 0.1 seconds, you reload just once for all of them, instead of 1000 times. – Charles Srstka Jul 24 '18 at 21:19
  • @CharlesSrstka I get what you are saying but the I don't think the ui is the problem bc I also tried it with reloading only about 4 times in the code block -> same result – John Smith Jul 24 '18 at 21:21
  • @Paulw11 I don't know anything about core Data, but it sounds like a lot of experience and time to implement – John Smith Jul 24 '18 at 21:24
  • 1
    Not really. It is pretty simple for what you want. There are a number of tutorials available. Create your Person entity in Core Data and it will write a lot of the code for you. However, even searching an array of only 6000 items shouldn't take very long. I don't understand why you are iterating over the search text letter by letter and then searching separately by the substrings. A typical name search would be "firstName starts with searchString or lastName starts with searchString". You can use "contains" rather than startsWith if you prefer. – Paulw11 Jul 24 '18 at 21:28
  • I think you have a [XY problem](https://en.wikipedia.org/wiki/XY_problem) - You shouldn't need to cancel an in-memory search if it is implemented efficiently – Paulw11 Jul 24 '18 at 21:29
  • @Paulw11 the reason im not just saying if starts with xxx is that I wanted to do it more advanced like InstantSearch iOS, which is server based, so that it can find even if you mistype – John Smith Jul 24 '18 at 21:33
  • @JohnSmith You can do things like regex searches with Core Data. – Charles Srstka Jul 24 '18 at 21:34
  • That is a [different strategy](https://en.wikipedia.org/wiki/Soundex). I saw the video that you posted and your search for "leonard" produces a result "toledor" or something just because it contains "leonard"; Producing too many results is possibly worse than too few. Regardless from your video the results seemed to come back quickly. Why are you dispatching the task at all? It seems that even running on the main queue it should be pretty fast with only 6000 candidates – Paulw11 Jul 24 '18 at 21:36
  • @Paulw11 It is really fast (fast enough), but if you type normally, while you type two characters, it completes one so the old one always adds the old (wrong) results to your array/table – John Smith Jul 24 '18 at 21:40
  • If your search is fast then you shouldn't notice the "incorrect" results, because the delay between typing a character and updating the results should be a tenth of a second or so. I would start by simplifying that search code; Just use startsWith for the two name components and see if that is working correctly, and then try and add features. Your current code is O(n^2). If you get your search right then there should be no need to dispatch or cancel or any of that stuff. – Paulw11 Jul 24 '18 at 21:54
  • Yes I know it works if I just say starts with, but thats totally not what I want – John Smith Jul 24 '18 at 21:55

3 Answers3

3

Your problem is, essentially, that you're checking global state when you should be checking local state. Let's say you've got operation 'L' going on, and you've just typed an 'e', so operation 'Le' is going to start. Here's what roughly happens:

func updateMyPeepz() {
    // At this point, `self.workItem` contains operation 'L'

    self.workItem.cancel() // just set isCancelled to true on operation 'L'
    self.workItem = DispatchWorkItem { /* bla bla bla */ }
    // *now* self.workItem points to operation 'Le'!
}

So later, in the work item for operation 'L', you do this:

if self.workItem.isCancelled { /* do something */ }

Unfortunately, this code is running in operation 'L', but self.workItem now points to operation 'Le'! So while operation 'L' is cancelled, and operation 'Le' is not, operation 'L' sees that self.workItem—i.e. operation 'Le'—is not cancelled. And thus, the check always returns false, and operation 'L' never actually stops.

When you use the global boolean variable, you have the same problem, because it's global and doesn't differentiate between the operations that should still be running and the ones that shouldn't (in addition to atomicity issues you're already going to introduce if you don't protect the variable with a semaphore).

Here's how to fix it:

func updateMyPeepz() {
    var workItem: DispatchWorkItem? = nil // This is a local variable

    self.currentWorkItem?.cancel() // Cancel the one that was already running

    workItem = DispatchWorkItem { /* bla */ } // Set the *local variable* not the property

    self.currentWorkItem = workItem // now we set the property
    DispatchQueue.global(qos: whatever).async(execute: workItem) // run it
}

Now, here's the crucial part. Inside your loops, check like this:

if workItem?.isCancelled ?? false // check local workItem, *not* self.workItem!
// (you can also do workItem!.isCancelled if you want, since we can be sure this is non-nil)

At the end of the workItem, set workItem to nil to get rid of the retain cycle (otherwise it'll leak):

workItem = nil // not self.workItem! Nil out *our* item, not the property

Alternatively, you can put [weak workItem] in at the top of the workItem block to prevent the cycle—then you don't have to nil it out at the end, but you should be sure to use ?? false instead of ! since you always want to assume that a weak variable can conceivably go nil at any time.

Charles Srstka
  • 16,665
  • 3
  • 34
  • 60
  • First impression is works a lot better, but now I got a lot of other errors like: tableView-cellForRow: searchedPeople?[indexPath.row] crashes (index out of range) – John Smith Jul 24 '18 at 22:15
  • @JohnSmith That's a new problem; probably something's getting queued to the main queue, then one of the subsequent searches is eliminating results from the table so there are fewer rows than the already-queued operation expects. I'd spend a bit of time trying to debug it on your own (doing a check of `workItem.isCancelled` inside the block you submit to the main queue might fix it), and post a new question if you still need help. – Charles Srstka Jul 24 '18 at 22:19
  • 1
    Yes I will figure this one out myself, but thanks for all your help and time <3 – John Smith Jul 24 '18 at 22:23
1

Edit: The 10 second delay is most likely due to some issue updating the UI. I just tested updating a shared boolean between the main queue and a dispatch work item on a background queue, and there was no apparent propagation delay of that magnitude. However, there's debate over whether this is safe here and here.

The self.tableView.reloadData() happening in the async blocks of the work item and also in the sync part before the work item... I'm not sure what behavior that'll create since you're relying on the order of the reloadData calls in the GCD queues behind the scenes. It'd be more predictable if your work items just built a local array of results and didn't semi-directly interact with the UI.

One proposal, not necessarily the best cause it's been a while since I've used Dispatch: Have your work items find the array of results then update a shared [String: [Person]] dict mapping search strings to result arrays (lock needed? not sure) when they're done. Then you could use DispatchWorkItem.notify (example) to run code on the main queue that updates the UI table when a work item finishes, using the shared dictionary result that matches the current typed search string or doing nothing if no result matches.

P.S. This is a lot of manual work, and I might be missing an easier way. I know CoreData automates the task of searching and updating a UI table with its NSFetchedResultsController, but that's a whole different setup.

sudo
  • 5,604
  • 5
  • 40
  • 78
  • 2
    In addition to this, you can probably improve performance by using `reloadData(forRowIndexes:columnIndexes:)` instead of `reloadData()` to only reload the rows of the table view containing data that's actually changed. – Charles Srstka Jul 24 '18 at 19:46
  • You didn't mention anything about my key problem: canceling the code exec. Making it faster with improvements like separating UI and searching is nice but doesn't fix my main problem. And the reloadTable is called so often bc I want to display the result instantly and not when all results are finished – John Smith Jul 24 '18 at 20:16
  • 1
    It's not about making it faster but fixing your issues with updating the UI. It sounds like you're cancelling the operation properly but seeing the UI respond in an unexpected way; a 10 second delay sounds like a UI problem, not a locking problem. Are you sure the problem is just the work item's function not seeing `stop` set to true when the other thread already set it true? You can check reasonably accurately with print statements, which I did try myself and saw it working without a delay. – sudo Jul 24 '18 at 20:28
  • @sudo Yeah, that makes more sense than anything else. Probably there aren't any delays in the work item itself, but rather the UI's getting overwhelmed by a huge number of reload requests. Think about it; if there's 3000 people in the database, the main queue will get sent 3000 reload requests, in a really short amount of time. That'll all have to complete before the UI can be responsive again. And especially since you're reloading the entire table every time, it could conceivably take a while for all those reloads to complete, especially if your data source methods aren't ideally efficient. – Charles Srstka Jul 24 '18 at 20:51
  • @sudo Thats exactly what I did. If the if statement was true, I printed "Cancelled + the string it just checked". If I typed "peter" it was still printing some reults for "p" a couple seconds after and I also saw those old results in my table. – John Smith Jul 24 '18 at 20:57
  • @CharlesSrstka First of all the table doesn't get reloaded 3000 times, bc it will only be reloaded when a result is added and its not just the ui bc I printed it out in the log 10 sec later so I know – John Smith Jul 24 '18 at 20:59
  • @JohnSmith Try sticking a `print` statement inside the `async` block to the main thread to see how many times it gets called. – Charles Srstka Jul 24 '18 at 21:03
  • @JohnSmith Also, try commenting out the call to the main queue altogether and see if it affects execution time. – Charles Srstka Jul 24 '18 at 21:05
  • @JohnSmith Finally, how many results for 'p' did it print? Dumping a really large number of logs to the console all at once is _itself_ an operation that can take some time to complete. – Charles Srstka Jul 24 '18 at 21:08
  • @CharlesSrstka I know that printing out a lot of times can freeze the log a little, but I also see it in the array that those old results are still getting added – John Smith Jul 24 '18 at 21:10
  • @JohnSmith Unfortunately, without either seeing some code that reproduces the problem or getting answers to some of the questions I asked above, there's not much to go on from here. – Charles Srstka Jul 24 '18 at 21:12
  • 1
    I'm guessing you already tried this, but just in case, the way I tested was: print the value of `stop` every time it's checked, and print "setting stop = true" when the main thread sets it true. I didn't print anything else. The result was many "stop: false" prints, one "setting stop = true" print, then many "stop: true" prints. – sudo Jul 24 '18 at 21:15
  • @JohnSmith Where's the cancel in that video? – Charles Srstka Jul 24 '18 at 21:21
  • @CharlesSrstka Stop variable that I set to false/true – John Smith Jul 24 '18 at 21:22
  • @JohnSmith Yeah, but what I mean is, when does it actually get set to true? – Charles Srstka Jul 24 '18 at 21:24
  • @CharlesSrstka Top of the video above the green, the video is not perfect so you don't really see it – John Smith Jul 24 '18 at 21:25
  • @JohnSmith Also, how many of these things do you have running at once? Whenever you detect `stop` in a loop, you set it to `false`, so if there's more than one work item going on, they're going to uncancel each other. Much better to use the work item's `isCancelled` method since that's not shared. – Charles Srstka Jul 24 '18 at 21:26
  • @JohnSmith I see a `stop = true` at the top, but it's commented out. – Charles Srstka Jul 24 '18 at 21:27
  • @CharlesSrstkaforget the work item, I removed it bc it didn't work any better than just a global variable and a async exec – John Smith Jul 24 '18 at 21:27
  • @JohnSmith That global variable is what's causing the behavior I'm seeing in the video. You set it to `true` at the beginning, but then you've got a bunch of searches going on; "L", "Le", "Leo", etc (you can see this in the logs). When any one of these detects that `stop` is `true`, it immediately sets `stop` back to `false` before breaking the loop. What this means is that you will only cancel one of the searches. Whichever one notices `stop` is `true` first will get cancelled, then the rest will keep going because `stop` is now `false`. – Charles Srstka Jul 24 '18 at 21:30
  • @JohnSmith I can't guarantee there aren't any _other_ problems in there (as Paul says, you _should_ make this more efficient), but the behavior shown in the video will be solved if you get rid of `stop` and use the work item's `isCancelled` property instead. – Charles Srstka Jul 24 '18 at 21:31
  • @CharlesSrstka Trust me I already tried it. If you're 100% sure, I can retest but I don't think thats the solution – John Smith Jul 24 '18 at 21:34
  • @JohnSmith I'm 100% sure that the specific problem seen in the video is caused by the global variable, and that it's obscuring whatever other problem(s) may exist, and making it impossible to go any further. Think about it. You have parallel searches going on at the same time that are setting `stop` to false when it becomes true. How's that going to affect the other searches? – Charles Srstka Jul 24 '18 at 21:35
  • @JohnSmith Here's how I'd do what I think you're trying to do: Get rid of the `stop` boolean, and store the work item as a property. When `textDidChange` gets fired, 1) cancel `self.workItem` if it's non-nil, and 2) reassign `self.workItem` to the new one you create. This way, the no longer relevant search for 'L' will stop once you type 'Le', but the 'Le' search will keep going (until you type 'Leo', at which time _it_ will cancel). – Charles Srstka Jul 24 '18 at 21:41
  • @JohnSmith Don't check `self.workItem.isCancelled`. `self.workItem` gets reassigned every time a new one of these starts, so this is the same as the global boolean. Instead, keep a _local_ variable to the work item inside the `textDidChange` method's scope. Then, you'll be checking whether _this specific_ item's been cancelled, instead of the latest work item (which will always be non-cancelled). – Charles Srstka Jul 24 '18 at 21:48
  • This is getting hard to do in comments, so I'm going to write a new answer. One moment. – Charles Srstka Jul 24 '18 at 21:50
  • @CharlesSrstka So I should never check for .isCancelled, just reassign workitem? – John Smith Jul 24 '18 at 21:50
  • @JohnSmith No. Check .isCancelled on the local variable. I'm writing an answer. One moment. – Charles Srstka Jul 24 '18 at 21:50
0

I am not sure that cancelling the operation is your real issue. Your search code is pretty inefficient. Refactoring so that the outer loop is your people array rather than your search string will make the operation much faster.

You can also compute a lot of values once.

Using the code below I was able to find 1438 matches from 24636 candidates in 0.32 seconds (or 0.23 seconds if I commented out the print "Found:...").

func search(_ people: [Person], for searchText: String) {
        guard !people.isEmpty  else {
            print("No people")
            return
        }

        let lcSearch = searchText.lowercased()

        print("Starting search for \(searchText)")
        var searchedPeople = [Person]()
        for person in people {
            let personName = ((person.firstName ?? "") + " " + (person.surname ?? "")).lowercased()
            if personName.contains(lcSearch) {
                searchedPeople.append(person)
                continue
            }
            for i in 1..<lcSearch.count {
                let split = lcSearch.index(lcSearch.startIndex, offsetBy: i)
                let firstP = lcSearch.prefix(upTo: split)
                let lastP = lcSearch.suffix(from: split)

                if personName.contains(firstP) && personName.contains(lastP) {
                    print("Found: \(personName), \(searchText), First: \(firstP), Last: \(lastP)")
                    searchedPeople.append(person)
                    break
                }
            }
        }

        print("Done.  Found \(searchedPeople.count)")
    }
Paulw11
  • 108,386
  • 14
  • 159
  • 186
  • I had it like this before where I would run the person for loop as outter and the other as inner, but the reason I switched was bc I want the exacts hits to show first – John Smith Jul 25 '18 at 08:14
  • And the time is fast, but never fast enough if you type normally between the keyboard hits – John Smith Jul 25 '18 at 08:20