Skip to content
This repository has been archived by the owner on Mar 8, 2024. It is now read-only.

Increase speed of fetching RSS feeds #11

Closed
wants to merge 3 commits into from
Closed

Conversation

calebhearth
Copy link

Benchmarks for before and after the code change are available in individual
commits. This change results in an average time of 802604696.33ns for warm
requests, decreased from 1806248432ns. This is a 55.6% improvement.

@@ -39,6 +38,18 @@ func rssHandler(rw http.ResponseWriter, r *http.Request) {
fmt.Fprintln(rw, result)
}

func fetchFeeds(master *feeds.Feed) {
var wg sync.WaitGroup
wg.Add(len(sourceFeeds))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though technically correct, its more common to call wg.Add(1) inside the for loop, just before the go func(...) call.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aight.

@bernerdschaefer
Copy link
Contributor

Did you run this with -race (https://golang.org/doc/articles/race_detector.html)? There are a few things which don't seem likely to be safe with concurrency, like calling master.Add(item) from multiple goroutines.

@croaky
Copy link
Contributor

croaky commented Oct 2, 2015

Is -race another thing we can add to CI somewhere?

@calebhearth
Copy link
Author

Did you run this with -race (https://golang.org/doc/articles/race_detector.html)? There are a few things which don't seem likely to be safe with concurrency, like calling master.Add(item) from multiple goroutines.

Totally right. I get WARNING: DATA RACE.

@bernerdschaefer pointed out that I should only be doing the fetching, not the merging into master. Since that'll require cross-thread communication I'll need to brush up on channel. Which I'll get to sometime next week probably.

@calebhearth
Copy link
Author

And I'll be sure to add -race to the circle.yml

On Oct 2, 2015, at 4:48 PM, Dan Croak notifications@github.com wrote:

Is -race another thing we can add to CI somewhere?


Reply to this email directly or view it on GitHub.

for _, feed := range sourceFeeds {
fetch(feed, master)
}
fetchFeeds(master)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the number of feeds is defined waitgroup may not be needed. Could just loop and grab from channel?

itemChan := make(chan *feeds.Item)

for _, feed := range sourceFeeds {
  go fetch(feed, itemChan)
}

for i := 1; i < len(sourceFeeds); i++ {  
  master.Add(<-itemChan)
}

And then thread the itemChan through the fetch instead of master

func fetch(feed sourceFeed, itemChan chan *feeds.Item) {
itemChan <- &feeds.Item{
  Title:       stripPodcastEpisodePrefix(items[i].Title),
  Link:        &feeds.Link{Href: items[i].Links[0].Href},
  Description: items[i].Description,
  Author:      &feeds.Author{Name: sourceName},
  Created:     published,
}

A few runs of the benchmark using `go test -bench=.`:

    BenchmarkFetchFeeds-8              1        2439212195 ns/op
    ok          github.com/thoughtbot/rss       2.451s
    BenchmarkFetchFeeds-8              1        1697722256 ns/op
    ok          github.com/thoughtbot/rss       1.709s
    BenchmarkFetchFeeds-8              1        1883837937 ns/op
    ok          github.com/thoughtbot/rss       1.895s
    BenchmarkFetchFeeds-8              1        1837185103 ns/op
    ok          github.com/thoughtbot/rss       1.848s
A few more benchmark results (still using `go test -bench=.`):

    BenchmarkFetchFeeds-8              1        1030948675 ns/op
    ok          github.com/thoughtbot/rss       1.044s
    BenchmarkFetchFeeds-8              2         699606858 ns/op
    ok          github.com/thoughtbot/rss       2.330s
    BenchmarkFetchFeeds-8              2         689556591 ns/op
    ok          github.com/thoughtbot/rss       2.248s
    BenchmarkFetchFeeds-8              1        1477364497 ns/op
    ok          github.com/thoughtbot/rss       1.489s
    BenchmarkFetchFeeds-8              1        1017372924 ns/op
    ok          github.com/thoughtbot/rss       1.029s
    BenchmarkFetchFeeds-8              2         931727308 ns/op
    ok          github.com/thoughtbot/rss       2.622s
@croaky
Copy link
Contributor

croaky commented Jan 11, 2016

When I check out this branch and view it in browser, I don't see any feeds.

croaky added a commit that referenced this pull request Jan 20, 2016
croaky added a commit that referenced this pull request Jan 20, 2016
croaky added a commit that referenced this pull request Jan 20, 2016
@calebhearth calebhearth closed this Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants