2

I have a vector of pointers, pointing to approx 10MB of packets. In that, from first 2MB, I wanna delete all those that matches my predicate. The problem here is remove_if iterates through the whole vector, even though its not required in my use case. Is there any other efficient way?

fn_del_first_2MB
{
    uint32 deletedSoFar = 0;
    uint32 deleteLimit = 2000000;

    auto it = std::remove_if (cache_vector.begin(), cache_vector.end(),[deleteLimit,&deletedSoFar](const rc_vector& item){
    if(item.ptr_rc->ref_count <= 0) {
        if (deletedSoFar < deleteLimit) {
            deletedSoFar += item.ptr_rc->u16packet_size;
        delete(item.ptr_rc->packet);    
        delete(item.ptr_rc);
            return true;
        }
        else    
            return false;
    }
    else
        return false;
    });
    cache_vector.erase(it, cache_vector.end());
}

In the above code, once the deletedSoFar is greater than deleteLimit, any iteration more than that is unwanted.

Rama
  • 3,222
  • 2
  • 11
  • 26
Novajik
  • 47
  • 6
  • 1
    Instead of `cache_vector.end()` put your own marker. There are different [std::remove_if](http://en.cppreference.com/w/cpp/algorithm/remove) overloads. –  Dec 20 '16 at 19:55
  • @RawN ^ That might be an appropriate answer with a concise code sample. – πάντα ῥεῖ Dec 20 '16 at 19:58
  • @πάνταῥεῖ Eer I lack the *concise code sample* part. I am only half way through the Meyers first book. –  Dec 20 '16 at 20:03
  • 1
    You can use something else than `cache_vector.end()` for the end of the range `std::remove_if()` should work on. Note, however, that `std::remove_if()` only rearranges the content and everything between the return from `std::remove_if()` and the end position has unspecified content. You'll probably want to follow up the operation with something moving the tail forward and then `erase()`ing everything between the tail and the end. – Dietmar Kühl Dec 20 '16 at 20:03
  • @Dietmar ^ Same for you. I think the question is _on-topic_ and that's a considerable answer. – πάντα ῥεῖ Dec 20 '16 at 20:06
  • @πάνταῥεῖ: ... but I stopped reading Meyers's STL book after a few items! ;-) – Dietmar Kühl Dec 20 '16 at 20:07
  • @Dietmar Good to know, not all of us are doing _cargo cult_ based coding ;-). – πάντα ῥεῖ Dec 20 '16 at 20:09
  • The one I am reading is the *Effective* one. –  Dec 20 '16 at 20:11
  • Re: "remove_if iterates through the whole vector" -- no, it only does that if you tell it to. `remove_if`, like all algorithms, takes a pair of iterators that define the range that it will examine. If you don't want to go through the whole vector, don't pass iterators that go through the whole vector. – Pete Becker Dec 20 '16 at 21:11
  • Can't you have smart pointer for `item.ptr_rc` to simplify the code ? – Jarod42 Dec 20 '16 at 22:17

3 Answers3

3

Instead of cache_vector.end() put your own iterator marker myIter. With the remove_if option you should follow the erase-remove idiom. Here is an example that affects only the first 4 elements:

#include <iostream>
#include <vector>
#include <algorithm>

int main()
{
    std::vector<int> vec = { 1, 2, 3, 4, 5, 6, 7, 8, 9 };
    size_t index = 4; // index is something you need to calculate
    auto myIter = vec.begin() + index; // Your iterator instead of vec.end()
    vec.erase(std::remove_if(vec.begin(), myIter, [](int x){return x < 3; }), myIter);
    // modified vector:
    for (const auto& a : vec)
    {
        std::cout << a << std::endl;
    }
    return 0;
}
  • Thanks, but in this case, myIter needs to be calculate at runtime, which means I need to parse once just to find myIter, and then do the actual erase-remove. – Novajik Dec 22 '16 at 08:26
1

There is no need for std::remove_if() to pass the .end() iterator as the second argument: as long as the first argument can reach the second argument by incrementing, any iterators can be passed.

There is somewhat of a complication as your condition depends on the accumulated size of the elements encountered so far. As it turns out, it looks as if std::remove_if() won't be used. Something like this should work (although I'm not sure if this use of std::find_if() is actually legal as it keeps changing the predicate):

std::size_t accumulated_size(0u);
auto send(std::find_if(cache_vector.begin(), cache_vector.end(),
                              [&](rc_vector const& item) {
        bool rc(accumulated_size < delete_limit);
        accumulated_size += item.ptr_rc->u16packet_size;
        return rc;
    });
std::for_each(cache_vector.begin(), send, [](rc_vector& item) {
       delete(item.ptr_rc->packet);    
       delete(item.ptr_rc);
    });
cache_vector.erase(cache_vector.begin(), send);

The std::for_each() could be folded into the use of std::find_if() as well but I prefer to keep things logically separate. For a sufficiently large sequence there could be a performance difference when the memory needs to be transferred to the cache twice. For the tiny numbers quoted I doubt that the difference can be measured.

Dietmar Kühl
  • 150,225
  • 13
  • 225
  • 380
  • You forget the `if (item.ptr_rc->ref_count <= 0)` condition of the OP. And so the `erase` is more complicated. – Jarod42 Dec 20 '16 at 22:15
1

You may use your own loop:

void fn_del_first_2MB()
{
    const uint32 deleteLimit = 2000000;

    uint32 deletedSoFar = 0;
    auto dest = cache_vector.begin();
    auto it = dest

    for (; it != cache_vector.end(); ++it) {
        const auto& item = *it;

        if (item.ptr_rc->ref_count <= 0) {
            deletedSoFar += item.ptr_rc->u16packet_size;
            delete(item.ptr_rc->packet);    
            delete(item.ptr_rc);
            if (deletedSoFar >= deleteLimit) {
                ++it;
                break;
            }
        } else if (dest != it) {
            *dest = std::move(*it);
            ++dest;
        }
    }
    cache_vector.erase(dest, it);
}
Jarod42
  • 203,559
  • 14
  • 181
  • 302
  • This seems to be an optimal solution, but when I try I end up in some other issue. However, thanks for your time. ` backtrace: /system/lib/libc.so (je_arena_dalloc_bin_locked+221) /system/lib/libc.so (je_tcache_bin_flush_small+238) /system/lib/libc.so (ifree+448) /system/lib/libc.so (free+10) /system/lib/smu/libwlan_core.so (_ZN15roam_scan_cache23roam_cache_delete_frontEj+220) /system/lib/smu/libwlan_core.so (_ZN15roam_scan_cache21roam_packet_processorEPv+660) /system/lib/libc.so (_ZL15__pthread_startPv+30) /system/lib/libc.so (__start_thread+6) ` – Novajik Dec 21 '16 at 13:45
  • 1
    @Ananth: I think I miss a `++it` just before `break;` (edited) – Jarod42 Dec 21 '16 at 14:38