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)