From 7c3c8dcae06e91dc11b2a8e69b13122e44381ddb Mon Sep 17 00:00:00 2001 From: Azareal Date: Sun, 1 Mar 2020 16:22:43 +1000 Subject: [PATCH] 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 | 33 +++++----- common/counters/systems.go | 2 +- common/topic_list.go | 132 +++++++++++++++++++++++++++++-------- main.go | 2 +- routes/forum.go | 76 ++------------------- 5 files changed, 124 insertions(+), 121 deletions(-) diff --git a/common/counters/routes.go b/common/counters/routes.go index 1fcdba06..260ef7a6 100644 --- a/common/counters/routes.go +++ b/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? - b.Lock() - b.counter++ - b.Unlock() + atomic.AddInt64(&b.counter, 1) } // 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 diff --git a/common/counters/systems.go b/common/counters/systems.go index d8d310ae..ac13b896 100644 --- a/common/counters/systems.go +++ b/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 } diff --git a/common/topic_list.go b/common/topic_list.go index f0bccb6e..8c0208cd 100644 --- a/common/topic_list.go +++ b/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), - evenGroups: make(map[int]*TopicListHolder), + oddGroups: 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) - return topicList, filteredForums, paginator, err + topicList, pagi, err = tList.getList(page, orderby, topicCount, argList, qlist) + 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) - return topicList, forumList, paginator, err + topicList, pagi, err = tList.getList(page, orderby, topicCount, argList, qlist) + 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("topic.PostCount == 2 && rlen < rcap: ", topic.PostCount == 2 && rlen < rcap) + //log.Print("t.PostCount: ", t.PostCount) + //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 t, err := tcache.Get(t.ID); err == nil { + if tc != 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 _, err := tcache.Get(t.ID); err == sql.ErrNoRows { - _ = tcache.Set(t.Topic()) + if tc != nil { + if _, err := tc.Get(t.ID); err == sql.ErrNoRows { + _ = 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 { - topic.Creator = userList[topic.CreatedBy] - topic.LastUser = userList[topic.LastReplyBy] + for _, t := range topicList { + t.Creator = userList[t.CreatedBy] + t.LastUser = userList[t.LastReplyBy] } pageList := Paginate(page, lastPage, 5) diff --git a/main.go b/main.go index 6e221fe1..d468dc7d 100644 --- a/main.go +++ b/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) } diff --git a/routes/forum.go b/routes/forum.go index cd842242..d868c114 100644 --- a/routes/forum.go +++ b/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 - 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() + topicList, pagi, err := c.TopicList.GetListByForum(forum, page, "") 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) - pi := c.ForumPage{header, topicList, forum, c.Paginator{pageList, page, lastPage}} + //pageList := c.Paginate(page, lastPage, 5) + pi := c.ForumPage{header, topicList, forum, pagi} tmpl := forum.Tmpl if tmpl == "" { ferr = renderTemplate("forum", w, r, header, pi)