unify single forums in topic lists and forum pages with GetListByForum

add GetListByForum method
remove forums with topic counts of zero from filter lists
avoid locking whenever possible in route view counter

common/counters/routes.go

@@ -3,6 +3,7 @@ package counters
 import (
 	"database/sql"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	c "github.com/Azareal/Gosora/common"
@@ -14,7 +15,7 @@ import (
 var RouteViewCounter *DefaultRouteViewCounter
 
 type RVBucket struct {
-	counter int
+	counter int64
 	avg     int
 
 	sync.Mutex
@@ -49,17 +50,16 @@ func NewDefaultRouteViewCounter(acc *qgen.Accumulator) (*DefaultRouteViewCounter
 
 type RVCount struct {
 	RouteID int
-	Count   int
+	Count   int64
 	Avg     int
 }
 
 func (co *DefaultRouteViewCounter) Tick() (err error) {
 	var tb []RVCount
 	for routeID, b := range co.buckets {
-		var count, avg int
+		var avg int
+		count := atomic.SwapInt64(&b.counter, 0)
 		b.Lock()
-		count = b.counter
-		b.counter = 0
 		avg = b.avg
 		b.avg = 0
 		b.Unlock()
@@ -96,9 +96,9 @@ func (co *DefaultRouteViewCounter) Tick() (err error) {
 	return nil
 }
 
-func (co *DefaultRouteViewCounter) insertChunk(count, avg, route int) error {
+func (co *DefaultRouteViewCounter) insertChunk(count int64, avg, route int) error {
 	routeName := reverseRouteMapEnum[route]
-	c.DebugLogf("Inserting a vchunk with a count of %d, avg of %d for route %s (%d)", count, avg, routeName, route)
+	c.DebugLogf("Inserting vchunk with count %d, avg %d for route %s (%d)", count, avg, routeName, route)
 	_, err := co.insert.Exec(count, avg, routeName)
 	return err
 }
@@ -109,9 +109,9 @@ func (co *DefaultRouteViewCounter) insert5Chunk(rvs []RVCount) error {
 	for _, rv := range rvs {
 		routeName := reverseRouteMapEnum[rv.RouteID]
 		if rv.Avg == 0 {
-			c.DebugLogf("Queueing a vchunk with a count of %d for routes %s (%d)", rv.Count, routeName, rv.RouteID)
+			c.DebugLogf("Queueing vchunk with count %d for routes %s (%d)", rv.Count, routeName, rv.RouteID)
 		} else {
-			c.DebugLogf("Queueing a vchunk with count %d, avg %d for routes %s (%d)", rv.Count, rv.Avg, routeName, rv.RouteID)
+			c.DebugLogf("Queueing vchunk with count %d, avg %d for routes %s (%d)", rv.Count, rv.Avg, routeName, rv.RouteID)
 		}
 		args[i] = rv.Count
 		args[i+1] = rv.Avg
@@ -129,14 +129,11 @@ func (co *DefaultRouteViewCounter) Bump(route int) {
 	}
 	// TODO: Test this check
 	b := co.buckets[route]
-	c.DebugDetail("buckets[", route, "]: ", b)
+	c.DebugDetail("bucket ", route, ": ", b)
 	if len(co.buckets) <= route || route < 0 {
 		return
 	}
-	// TODO: Avoid lock by using atomic increment?
+	atomic.AddInt64(&b.counter, 1)
-	b.Lock()
-	b.counter++
-	b.Unlock()
 }
 
 // TODO: Eliminate the lock?
@@ -146,14 +143,14 @@ func (co *DefaultRouteViewCounter) Bump2(route int, t time.Time) {
 	}
 	// TODO: Test this check
 	b := co.buckets[route]
-	c.DebugDetail("buckets[", route, "]: ", b)
+	c.DebugDetail("bucket ", route, ": ", b)
 	if len(co.buckets) <= route || route < 0 {
 		return
 	}
 	micro := int(time.Since(t).Microseconds())
 	//co.PerfCounter.Push(since, true)
+	atomic.AddInt64(&b.counter, 1)
 	b.Lock()
-	b.counter++
 	if micro != b.avg {
 		if b.avg == 0 {
 			b.avg = micro
@@ -171,14 +168,14 @@ func (co *DefaultRouteViewCounter) Bump3(route int, nano int64) {
 	}
 	// TODO: Test this check
 	b := co.buckets[route]
-	c.DebugDetail("buckets[", route, "]: ", b)
+	c.DebugDetail("bucket ", route, ": ", b)
 	if len(co.buckets) <= route || route < 0 {
 		return
 	}
 	micro := int((uutils.Nanotime() - nano) / 1000)
 	//co.PerfCounter.Push(since, true)
+	atomic.AddInt64(&b.counter, 1)
 	b.Lock()
-	b.counter++
 	if micro != b.avg {
 		if b.avg == 0 {
 			b.avg = micro

common/counters/systems.go

@@ -50,7 +50,7 @@ func (co *DefaultOSViewCounter) insertChunk(count int64, os int) error {
 
 func (co *DefaultOSViewCounter) Bump(id int) {
 	// TODO: Test this check
-	c.DebugDetail("buckets[", id, "]: ", co.buckets[id])
+	c.DebugDetail("bucket ", id, ": ", co.buckets[id])
 	if len(co.buckets) <= id || id < 0 {
 		return
 	}

common/topic_list.go

@@ -20,6 +20,7 @@ type TopicListHolder struct {
 type TopicListInt interface {
 	GetListByCanSee(canSee []int, page int, orderby string, filterIDs []int) (topicList []*TopicsRow, forumList []Forum, paginator Paginator, err error)
 	GetListByGroup(group *Group, page int, orderby string, filterIDs []int) (topicList []*TopicsRow, forumList []Forum, paginator Paginator, err error)
+	GetListByForum(f *Forum, page int, orderby string) (topicList []*TopicsRow, paginator Paginator, err error)
 	GetList(page int, orderby string, filterIDs []int) (topicList []*TopicsRow, forumList []Forum, paginator Paginator, err error)
 }
 
@@ -32,15 +33,21 @@ type DefaultTopicList struct {
 
 	//permTree atomic.Value // [string(canSee)]canSee
 	//permTree map[string][]int // [string(canSee)]canSee
+
+	getTopicsByForum *sql.Stmt
 }
 
 // We've removed the topic list cache cap as admins really shouldn't be abusing groups like this with plugin_guilds around and it was extremely fiddly.
 // If this becomes a problem later on, then we can revisit this with a fresh perspective, particularly with regards to what people expect a group to really be
 // Also, keep in mind that as-long as the groups don't all have unique sets of forums they can see, then we can optimise a large portion of the work away.
-func NewDefaultTopicList() (*DefaultTopicList, error) {
+func NewDefaultTopicList(acc *qgen.Accumulator) (*DefaultTopicList, error) {
 	tList := &DefaultTopicList{
-		oddGroups:  make(map[int]*TopicListHolder),
+		oddGroups:        make(map[int]*TopicListHolder),
-		evenGroups: make(map[int]*TopicListHolder),
+		evenGroups:       make(map[int]*TopicListHolder),
+		getTopicsByForum: acc.Select("topics").Columns("tid, title, content, createdBy, is_closed, sticky, createdAt, lastReplyAt, lastReplyBy, lastReplyID, views, postCount, likeCount").Where("parentID=?").Orderby("sticky DESC, lastReplyAt DESC, createdBy DESC").Limit("?,?").Prepare(),
+	}
+	if err := acc.FirstError(); err != nil {
+		return nil, err
 	}
 
 	err := tList.Tick()
@@ -121,6 +128,67 @@ func (tList *DefaultTopicList) Tick() error {
 	return nil
 }
 
+// TODO: Add Topics() method to *Forum?
+// TODO: Implement orderby
+func (tList *DefaultTopicList) GetListByForum(f *Forum, page int, orderby string) (topicList []*TopicsRow, paginator Paginator, err error) {
+	// TODO: Does forum.TopicCount take the deleted items into consideration for guests? We don't have soft-delete yet, only hard-delete
+	offset, page, lastPage := PageOffset(f.TopicCount, page, Config.ItemsPerPage)
+
+	rows, err := tList.getTopicsByForum.Query(f.ID, offset, Config.ItemsPerPage)
+	if err != nil {
+		return nil, Paginator{nil, 1, 1}, err
+	}
+	defer rows.Close()
+
+	// TODO: Use something other than TopicsRow as we don't need to store the forum name and link on each and every topic item?
+	reqUserList := make(map[int]bool)
+	for rows.Next() {
+		t := TopicsRow{ID: 0}
+		err := rows.Scan(&t.ID, &t.Title, &t.Content, &t.CreatedBy, &t.IsClosed, &t.Sticky, &t.CreatedAt, &t.LastReplyAt, &t.LastReplyBy, &t.LastReplyID, &t.ViewCount, &t.PostCount, &t.LikeCount)
+		if err != nil {
+			return nil, Paginator{nil, 1, 1}, err
+		}
+
+		t.ParentID = f.ID
+		t.Link = BuildTopicURL(NameToSlug(t.Title), t.ID)
+		// TODO: Create a specialised function with a bit less overhead for getting the last page for a post count
+		_, _, lastPage := PageOffset(t.PostCount, 1, Config.ItemsPerPage)
+		t.LastPage = lastPage
+
+		//header.Hooks.VhookNoRet("forum_trow_assign", &t, &forum)
+		topicList = append(topicList, &t)
+		reqUserList[t.CreatedBy] = true
+		reqUserList[t.LastReplyBy] = true
+	}
+	if err = rows.Err(); err != nil {
+		return nil, Paginator{nil, 1, 1}, err
+	}
+
+	// Convert the user ID map to a slice, then bulk load the users
+	idSlice := make([]int, len(reqUserList))
+	var i int
+	for userID := range reqUserList {
+		idSlice[i] = userID
+		i++
+	}
+
+	// TODO: What if a user is deleted via the Control Panel?
+	userList, err := Users.BulkGetMap(idSlice)
+	if err != nil {
+		return nil, Paginator{nil, 1, 1}, err
+	}
+
+	// Second pass to the add the user data
+	// TODO: Use a pointer to TopicsRow instead of TopicsRow itself?
+	for _, t := range topicList {
+		t.Creator = userList[t.CreatedBy]
+		t.LastUser = userList[t.LastReplyBy]
+	}
+
+	pageList := Paginate(page, lastPage, 5)
+	return topicList, Paginator{pageList, page, lastPage}, nil
+}
+
 func (tList *DefaultTopicList) GetListByGroup(group *Group, page int, orderby string, filterIDs []int) (topicList []*TopicsRow, forumList []Forum, paginator Paginator, err error) {
 	if page == 0 {
 		page = 1
@@ -148,13 +216,13 @@ func (tList *DefaultTopicList) GetListByGroup(group *Group, page int, orderby st
 	return tList.GetListByCanSee(group.CanSee, page, orderby, filterIDs)
 }
 
-func (tList *DefaultTopicList) GetListByCanSee(canSee []int, page int, orderby string, filterIDs []int) (topicList []*TopicsRow, forumList []Forum, paginator Paginator, err error) {
+func (tList *DefaultTopicList) GetListByCanSee(canSee []int, page int, orderby string, filterIDs []int) (topicList []*TopicsRow, forumList []Forum, pagi Paginator, err error) {
 	// TODO: Optimise this by filtering canSee and then fetching the forums?
 	// We need a list of the visible forums for Quick Topic
 	// ? - Would it be useful, if we could post in social groups from /topics/?
 	for _, fid := range canSee {
 		f := Forums.DirtyGet(fid)
-		if f.Name != "" && f.Active && (f.ParentType == "" || f.ParentType == "forum") {
+		if f.Name != "" && f.Active && (f.ParentType == "" || f.ParentType == "forum") && f.TopicCount != 0 {
 			fcopy := f.Copy()
 			// TODO: Add a hook here for plugin_guilds
 			forumList = append(forumList, fcopy)
@@ -180,6 +248,11 @@ func (tList *DefaultTopicList) GetListByCanSee(canSee []int, page int, orderby s
 	} else {
 		filteredForums = forumList
 	}
+	if len(filteredForums) == 1 && orderby == "" {
+		topicList, pagi, err = tList.GetListByForum(&filteredForums[0], page, orderby)
+		return topicList, forumList, pagi, err
+	}
+
 	var topicCount int
 	for _, f := range filteredForums {
 		topicCount += f.TopicCount
@@ -192,12 +265,12 @@ func (tList *DefaultTopicList) GetListByCanSee(canSee []int, page int, orderby s
 		return topicList, filteredForums, Paginator{[]int{}, 1, 1}, nil
 	}
 
-	topicList, paginator, err = tList.getList(page, orderby, topicCount, argList, qlist)
+	topicList, pagi, err = tList.getList(page, orderby, topicCount, argList, qlist)
-	return topicList, filteredForums, paginator, err
+	return topicList, filteredForums, pagi, err
 }
 
 // TODO: Reduce the number of returns
-func (tList *DefaultTopicList) GetList(page int, orderby string, filterIDs []int) (topicList []*TopicsRow, forumList []Forum, paginator Paginator, err error) {
+func (tList *DefaultTopicList) GetList(page int, orderby string, filterIDs []int) (topicList []*TopicsRow, forumList []Forum, pagi Paginator, err error) {
 	// TODO: Make CanSee a method on *Group with a canSee field? Have a CanSee method on *User to cover the case of superadmins?
 	cCanSee, err := Forums.GetAllVisibleIDs()
 	if err != nil {
@@ -229,13 +302,17 @@ func (tList *DefaultTopicList) GetList(page int, orderby string, filterIDs []int
 	var topicCount int
 	for _, fid := range canSee {
 		f := Forums.DirtyGet(fid)
-		if f.Name != "" && f.Active && (f.ParentType == "" || f.ParentType == "forum") {
+		if f.Name != "" && f.Active && (f.ParentType == "" || f.ParentType == "forum") && f.TopicCount != 0 {
 			fcopy := f.Copy()
 			// TODO: Add a hook here for plugin_guilds
 			forumList = append(forumList, fcopy)
 			topicCount += fcopy.TopicCount
 		}
 	}
+	if len(forumList) == 1 && orderby == "" {
+		topicList, pagi, err = tList.GetListByForum(&forumList[0], page, orderby)
+		return topicList, forumList, pagi, err
+	}
 
 	// ? - Should we be showing plugin_guilds posts on /topics/?
 	argList, qlist := ForumListToArgQ(forumList)
@@ -244,26 +321,23 @@ func (tList *DefaultTopicList) GetList(page int, orderby string, filterIDs []int
 		return topicList, forumList, Paginator{[]int{}, 1, 1}, err
 	}
 
-	topicList, paginator, err = tList.getList(page, orderby, topicCount, argList, qlist)
+	topicList, pagi, err = tList.getList(page, orderby, topicCount, argList, qlist)
-	return topicList, forumList, paginator, err
+	return topicList, forumList, pagi, err
 }
 
 // TODO: Rename this to TopicListStore and pass back a TopicList instance holding the pagination data and topic list rather than passing them back one argument at a time
+// TODO: Make orderby an enum of sorts
 func (tList *DefaultTopicList) getList(page int, orderby string, topicCount int, argList []interface{}, qlist string) (topicList []*TopicsRow, paginator Paginator, err error) {
 	//log.Printf("argList: %+v\n",argList)
 	//log.Printf("qlist: %+v\n",qlist)
-	/*topicCount, err := ArgQToTopicCount(argList, qlist)
-	if err != nil {
-		return nil, Paginator{nil, 1, 1}, err
-	}*/
-	offset, page, lastPage := PageOffset(topicCount, page, Config.ItemsPerPage)
 
 	var orderq string
 	if orderby == "most-viewed" {
-		orderq = "views DESC, lastReplyAt DESC, createdBy DESC"
+		orderq = "views DESC,lastReplyAt DESC,createdBy DESC"
 	} else {
-		orderq = "sticky DESC, lastReplyAt DESC, createdBy DESC"
+		orderq = "sticky DESC,lastReplyAt DESC,createdBy DESC"
 	}
+	offset, page, lastPage := PageOffset(topicCount, page, Config.ItemsPerPage)
 
 	// TODO: Prepare common qlist lengths to speed this up in common cases, prepared statements are prepared lazily anyway, so it probably doesn't matter if we do ten or so
 	stmt, err := qgen.Builder.SimpleSelect("topics", "tid,title,content,createdBy,is_closed,sticky,createdAt,lastReplyAt,lastReplyBy,lastReplyID,parentID,views,postCount,likeCount,attachCount,poll,data", "parentID IN("+qlist+")", orderq, "?,?")
@@ -284,7 +358,7 @@ func (tList *DefaultTopicList) getList(page int, orderby string, topicCount int,
 	rcache := Rstore.GetCache()
 	rcap := rcache.GetCapacity()
 	rlen := rcache.Length()
-	tcache := Topics.GetCache()
+	tc := Topics.GetCache()
 	reqUserList := make(map[int]bool)
 	for rows.Next() {
 		// TODO: Embed Topic structs in TopicsRow to make it easier for us to reuse this work in the topic cache
@@ -312,13 +386,13 @@ func (tList *DefaultTopicList) getList(page int, orderby string, topicCount int,
 
 		//log.Print("rlen: ", rlen)
 		//log.Print("rcap: ", rcap)
-		//log.Print("topic.PostCount: ", t.PostCount)
+		//log.Print("t.PostCount: ", t.PostCount)
-		//log.Print("topic.PostCount == 2 && rlen < rcap: ", topic.PostCount == 2 && rlen < rcap)
+		//log.Print("t.PostCount == 2 && rlen < rcap: ", t.PostCount == 2 && rlen < rcap)
 
 		// Avoid the extra queries on topic list pages, if we already have what we want...
 		hRids := false
-		if tcache != nil {
+		if tc != nil {
-			if t, err := tcache.Get(t.ID); err == nil {
+			if t, err := tc.Get(t.ID); err == nil {
 				hRids = len(t.Rids) != 0
 			}
 		}
@@ -338,9 +412,9 @@ func (tList *DefaultTopicList) getList(page int, orderby string, topicCount int,
 			t.Rids = []int{rids[0]}
 		}
 
-		if tcache != nil {
+		if tc != nil {
-			if _, err := tcache.Get(t.ID); err == sql.ErrNoRows {
+			if _, err := tc.Get(t.ID); err == sql.ErrNoRows {
-				_ = tcache.Set(t.Topic())
+				_ = tc.Set(t.Topic())
 			}
 		}
 	}
@@ -364,9 +438,9 @@ func (tList *DefaultTopicList) getList(page int, orderby string, topicCount int,
 
 	// Second pass to the add the user data
 	// TODO: Use a pointer to TopicsRow instead of TopicsRow itself?
-	for _, topic := range topicList {
+	for _, t := range topicList {
-		topic.Creator = userList[topic.CreatedBy]
+		t.Creator = userList[t.CreatedBy]
-		topic.LastUser = userList[topic.LastReplyBy]
+		t.LastUser = userList[t.LastReplyBy]
 	}
 
 	pageList := Paginate(page, lastPage, 5)

main.go

@@ -253,7 +253,7 @@ func storeInit() (err error) {
 	if err != nil {
 		return errors.WithStack(err)
 	}
-	c.TopicList, err = c.NewDefaultTopicList()
+	c.TopicList, err = c.NewDefaultTopicList(acc)
 	if err != nil {
 		return errors.WithStack(err)
 	}

routes/forum.go

@@ -8,25 +8,8 @@ import (
 	c "github.com/Azareal/Gosora/common"
 	"github.com/Azareal/Gosora/common/counters"
 	p "github.com/Azareal/Gosora/common/phrases"
-	qgen "github.com/Azareal/Gosora/query_gen"
 )
 
-type ForumStmts struct {
-	getTopics *sql.Stmt
-}
-
-var forumStmts ForumStmts
-
-// TODO: Move these DbInits into *Forum as Topics()
-func init() {
-	c.DbInits.Add(func(acc *qgen.Accumulator) error {
-		forumStmts = ForumStmts{
-			getTopics: acc.Select("topics").Columns("tid, title, content, createdBy, is_closed, sticky, createdAt, lastReplyAt, lastReplyBy, lastReplyID, views, postCount, likeCount").Where("parentID=?").Orderby("sticky DESC, lastReplyAt DESC, createdBy DESC").Limit("?,?").Prepare(),
-		}
-		return acc.FirstError()
-	})
-}
-
 // TODO: Retire this in favour of an alias for /topics/?
 func ViewForum(w http.ResponseWriter, r *http.Request, user c.User, header *c.Header, sfid string) c.RouteError {
 	page, _ := strconv.Atoi(r.FormValue("page"))
@@ -54,68 +37,17 @@ func ViewForum(w http.ResponseWriter, r *http.Request, user c.User, header *c.He
 	header.Title = forum.Name
 	header.OGDesc = forum.Desc
 
-	// TODO: Does forum.TopicCount take the deleted items into consideration for guests? We don't have soft-delete yet, only hard-delete
+	topicList, pagi, err := c.TopicList.GetListByForum(forum, page, "")
-	offset, page, lastPage := c.PageOffset(forum.TopicCount, page, c.Config.ItemsPerPage)
-
-	// TODO: Move this to *Forum
-	rows, err := forumStmts.getTopics.Query(fid, offset, c.Config.ItemsPerPage)
-	if err != nil {
-		return c.InternalError(err, w, r)
-	}
-	defer rows.Close()
-
-	// TODO: Use something other than TopicsRow as we don't need to store the forum name and link on each and every topic item?
-	var topicList []*c.TopicsRow
-	reqUserList := make(map[int]bool)
-	for rows.Next() {
-		t := c.TopicsRow{ID: 0}
-		err := rows.Scan(&t.ID, &t.Title, &t.Content, &t.CreatedBy, &t.IsClosed, &t.Sticky, &t.CreatedAt, &t.LastReplyAt, &t.LastReplyBy, &t.LastReplyID, &t.ViewCount, &t.PostCount, &t.LikeCount)
-		if err != nil {
-			return c.InternalError(err, w, r)
-		}
-
-		t.ParentID = fid
-		t.Link = c.BuildTopicURL(c.NameToSlug(t.Title), t.ID)
-		// TODO: Create a specialised function with a bit less overhead for getting the last page for a post count
-		_, _, lastPage := c.PageOffset(t.PostCount, 1, c.Config.ItemsPerPage)
-		t.LastPage = lastPage
-
-		header.Hooks.VhookNoRet("forum_trow_assign", &t, &forum)
-		topicList = append(topicList, &t)
-		reqUserList[t.CreatedBy] = true
-		reqUserList[t.LastReplyBy] = true
-	}
-	err = rows.Err()
 	if err != nil {
 		return c.InternalError(err, w, r)
 	}
 
-	// Convert the user ID map to a slice, then bulk load the users
-	idSlice := make([]int, len(reqUserList))
-	var i int
-	for userID := range reqUserList {
-		idSlice[i] = userID
-		i++
-	}
-
-	// TODO: What if a user is deleted via the Control Panel?
-	userList, err := c.Users.BulkGetMap(idSlice)
-	if err != nil {
-		return c.InternalError(err, w, r)
-	}
-
-	// Second pass to the add the user data
-	// TODO: Use a pointer to TopicsRow instead of TopicsRow itself?
-	for _, t := range topicList {
-		t.Creator = userList[t.CreatedBy]
-		t.LastUser = userList[t.LastReplyBy]
-	}
 	header.Zone = "view_forum"
 	header.ZoneID = forum.ID
 
 	// TODO: Reduce the amount of boilerplate here
 	if r.FormValue("js") == "1" {
-		outBytes, err := wsTopicList(topicList, lastPage).MarshalJSON()
+		outBytes, err := wsTopicList(topicList, pagi.LastPage).MarshalJSON()
 		if err != nil {
 			return c.InternalError(err, w, r)
 		}
@@ -123,8 +55,8 @@ func ViewForum(w http.ResponseWriter, r *http.Request, user c.User, header *c.He
 		return nil
 	}
 
-	pageList := c.Paginate(page, lastPage, 5)
+	//pageList := c.Paginate(page, lastPage, 5)
-	pi := c.ForumPage{header, topicList, forum, c.Paginator{pageList, page, lastPage}}
+	pi := c.ForumPage{header, topicList, forum, pagi}
 	tmpl := forum.Tmpl
 	if tmpl == "" {
 		ferr = renderTemplate("forum", w, r, header, pi)