Skip to main content
added 863 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

##Code Review

Lets stop using C-casts. They are hard to see and dangerous.

((std::string*)userp)->append((char*)contents, size * nmemb);

To make it obvious for code review please use the C++ casts. They emphasis how dangrerious they are and make them visable:

reinterpret_cast<std::string*>(userp)->append(static_cast<char*>(contents), size * nmemb);

##Code Review

Lets stop using C-casts. They are hard to see and dangerous.

((std::string*)userp)->append((char*)contents, size * nmemb);

To make it obvious for code review please use the C++ casts. They emphasis how dangrerious they are and make them visable:

reinterpret_cast<std::string*>(userp)->append(static_cast<char*>(contents), size * nmemb);
added 863 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

By simply removing this bottle neck I removed 5 seconds (averaged over three runs). Time to run was reduced from 18 to 13 seconds (timed externally). I simply made one thread do even cities and the other thread do odd cities (I know its not a perfect solution but I wanted do demonstrate the cost of threads being blocked by each other on such a coarse grain).

##Threading and Networking.

I would say that threading and networking is not the best way to demonstrate a good threading application.

The problem is that your thread spends an awful lot of its time blocked waiting for network to catch up and give it data. Usually what you do is have a single thread handling many connections simultaneously and use an even library to tell the thread when there is action on any of the sockets. I like libEvent for this it is one of the more modern ones.

But Curl has its own way of doing this with the "Multi Handle". You can create many curl handles and attach them to a multi handle then a single thread will watch all the handles simultaneously.

##Curl Handle re-use.

Like Edward mentioned (but worth re-mentioning). Reusing the same curl handle is essential to efficiency when connecting to the same site.

By simply removing this bottle neck I removed 5 seconds (averaged over three runs). I simply made one thread do even cities and the other thread do odd cities (I know its not a perfect solution but I wanted do demonstrate the cost of threads being blocked by each other on such a coarse grain).

By simply removing this bottle neck I removed 5 seconds (averaged over three runs). Time to run was reduced from 18 to 13 seconds (timed externally). I simply made one thread do even cities and the other thread do odd cities (I know its not a perfect solution but I wanted do demonstrate the cost of threads being blocked by each other on such a coarse grain).

##Threading and Networking.

I would say that threading and networking is not the best way to demonstrate a good threading application.

The problem is that your thread spends an awful lot of its time blocked waiting for network to catch up and give it data. Usually what you do is have a single thread handling many connections simultaneously and use an even library to tell the thread when there is action on any of the sockets. I like libEvent for this it is one of the more modern ones.

But Curl has its own way of doing this with the "Multi Handle". You can create many curl handles and attach them to a multi handle then a single thread will watch all the handles simultaneously.

##Curl Handle re-use.

Like Edward mentioned (but worth re-mentioning). Reusing the same curl handle is essential to efficiency when connecting to the same site.

fixed minor spelling error
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284
void fetch_weather_async()
{
    while (current_city + 1 < (int) cities.size()){
        // Say there is only one city left to fetch the data from.
        // And both threads have reached this lock at the same time
        // (multi processor system). One will get the lock the other
        // will stall waiting for the lock.
        city_counter_lock.lock();

        int city_index = 0;

        // The first thread threwthrough the lock will satisfy this
        // condition and get to processes the last city.
        // The second thread through the lock (after the first
        // releases it) will find this condition will fail and
        // as a result have `city_index = 0`
        if (current_city + 1 < (int) cities.size()) {
            city_index = ++current_city;
        }
        city_counter_lock.unlock();

        // Both threads now execute the following code.  
        // But one is set to processes the last item,
        // the other is going to re-processes the first item.
        std::cout << "requesting " << city_index << std::endl;  
        fetchJson(city_index);
        std::cout << "acquired " << city_index << std::endl; 

        // You have encountered your first race condition.   
    }
}
void fetch_weather_async()
{
    while (current_city + 1 < (int) cities.size()){
        // Say there is only one city left to fetch the data from.
        // And both threads have reached this lock at the same time
        // (multi processor system). One will get the lock the other
        // will stall waiting for the lock.
        city_counter_lock.lock();

        int city_index = 0;

        // The first thread threw the lock will satisfy this
        // condition and get to processes the last city.
        // The second thread through the lock (after the first
        // releases it) will find this condition will fail and
        // as a result have `city_index = 0`
        if (current_city + 1 < (int) cities.size()) {
            city_index = ++current_city;
        }
        city_counter_lock.unlock();

        // Both threads now execute the following code.  
        // But one is set to processes the last item,
        // the other is going to re-processes the first item.
        std::cout << "requesting " << city_index << std::endl;  
        fetchJson(city_index);
        std::cout << "acquired " << city_index << std::endl; 

        // You have encountered your first race condition.   
    }
}
void fetch_weather_async()
{
    while (current_city + 1 < (int) cities.size()){
        // Say there is only one city left to fetch the data from.
        // And both threads have reached this lock at the same time
        // (multi processor system). One will get the lock the other
        // will stall waiting for the lock.
        city_counter_lock.lock();

        int city_index = 0;

        // The first thread through the lock will satisfy this
        // condition and get to processes the last city.
        // The second thread through the lock (after the first
        // releases it) will find this condition will fail and
        // as a result have `city_index = 0`
        if (current_city + 1 < (int) cities.size()) {
            city_index = ++current_city;
        }
        city_counter_lock.unlock();

        // Both threads now execute the following code.  
        // But one is set to processes the last item,
        // the other is going to re-processes the first item.
        std::cout << "requesting " << city_index << std::endl;  
        fetchJson(city_index);
        std::cout << "acquired " << city_index << std::endl; 

        // You have encountered your first race condition.   
    }
}
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341
Loading