From 280be96d2b2997a3a4130634f5e9021eefdfa162 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 21 Feb 2023 22:38:39 +0000 Subject: [PATCH 1/4] Speed up HasUserStopwatch & GetActiveStopwatch GetActiveStopwatch & HasUserStopwatch is a hot piece of code that is repeatedly called and on examination of the cpu profile for TestGit it represents 0.44 seconds of CPU time. This PR reduces this time to 80ms. Signed-off-by: Andrew Thornton --- models/issues/stopwatch.go | 27 +++++++++++++++++++-------- routers/web/repo/issue.go | 11 +---------- routers/web/repo/issue_stopwatch.go | 18 +++--------------- 3 files changed, 23 insertions(+), 33 deletions(-) diff --git a/models/issues/stopwatch.go b/models/issues/stopwatch.go index 6bf936c5d41b1..f4fb2bcada03f 100644 --- a/models/issues/stopwatch.go +++ b/models/issues/stopwatch.go @@ -9,6 +9,7 @@ import ( "time" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" @@ -46,6 +47,7 @@ func (err ErrIssueStopwatchAlreadyExist) Unwrap() error { type Stopwatch struct { ID int64 `xorm:"pk autoincr"` IssueID int64 `xorm:"INDEX"` + Issue *Issue `xorm:"-"` UserID int64 `xorm:"INDEX"` CreatedUnix timeutil.TimeStamp `xorm:"created"` } @@ -133,10 +135,24 @@ func StopwatchExists(userID, issueID int64) bool { // HasUserStopwatch returns true if the user has a stopwatch func HasUserStopwatch(ctx context.Context, userID int64) (exists bool, sw *Stopwatch, err error) { - sw = new(Stopwatch) + type stopwatchIssueRepo struct { + Stopwatch `xorm:"extends"` + Issue `xorm:"extends"` + repo.Repository `xorm:"extends"` + } + + swIR := new(stopwatchIssueRepo) exists, err = db.GetEngine(ctx). + Table("stopwatch"). Where("user_id = ?", userID). - Get(sw) + Join("INNER", "issue", "issue.id = stopwatch.issue_id"). + Join("INNER", "repository", "repository.id = issue.repo_id"). + Get(swIR) + if exists { + sw = &swIR.Stopwatch + sw.Issue = &swIR.Issue + sw.Issue.Repo = &swIR.Repository + } return exists, sw, err } @@ -222,12 +238,7 @@ func CreateIssueStopwatch(ctx context.Context, user *user_model.User, issue *Iss return err } if exists { - issue, err := GetIssueByID(ctx, sw.IssueID) - if err != nil { - return err - } - - if err := FinishIssueStopwatch(ctx, user, issue); err != nil { + if err := FinishIssueStopwatch(ctx, user, sw.Issue); err != nil { return err } } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 05ba26a70cda6..7d9d5c7bc82c1 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1435,17 +1435,8 @@ func ViewIssue(ctx *context.Context) { ctx.Data["HasUserStopwatch"] = exists if exists { // Add warning if the user has already a stopwatch - var otherIssue *issues_model.Issue - if otherIssue, err = issues_model.GetIssueByID(ctx, sw.IssueID); err != nil { - ctx.ServerError("GetIssueByID", err) - return - } - if err = otherIssue.LoadRepo(ctx); err != nil { - ctx.ServerError("LoadRepo", err) - return - } // Add link to the issue of the already running stopwatch - ctx.Data["OtherStopwatchURL"] = otherIssue.Link() + ctx.Data["OtherStopwatchURL"] = sw.Issue.Link() } } ctx.Data["CanUseTimetracker"] = ctx.Repo.CanUseTimetracker(issue, ctx.Doer) diff --git a/routers/web/repo/issue_stopwatch.go b/routers/web/repo/issue_stopwatch.go index 3d20b08b49504..bbb751129cc99 100644 --- a/routers/web/repo/issue_stopwatch.go +++ b/routers/web/repo/issue_stopwatch.go @@ -96,22 +96,10 @@ func GetActiveStopwatch(ctx *context.Context) { return } - issue, err := issues_model.GetIssueByID(ctx, sw.IssueID) - if err != nil || issue == nil { - if !issues_model.IsErrIssueNotExist(err) { - ctx.ServerError("GetIssueByID", err) - } - return - } - if err = issue.LoadRepo(ctx); err != nil { - ctx.ServerError("LoadRepo", err) - return - } - ctx.Data["ActiveStopwatch"] = StopwatchTmplInfo{ - issue.Link(), - issue.Repo.FullName(), - issue.Index, + sw.Issue.Link(), + sw.Issue.Repo.FullName(), + sw.Issue.Index, sw.Seconds() + 1, // ensure time is never zero in ui } } From 763861603f97a261744830bc5b875345e73d36fd Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 22 Feb 2023 09:58:29 +0000 Subject: [PATCH 2/4] remove the issue holder from stopwatch Signed-off-by: Andrew Thornton --- models/issues/stopwatch.go | 13 ++++++------- models/issues/stopwatch_test.go | 4 ++-- routers/web/repo/issue.go | 6 +++--- routers/web/repo/issue_stopwatch.go | 8 ++++---- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/models/issues/stopwatch.go b/models/issues/stopwatch.go index f4fb2bcada03f..017a8ddefc6d2 100644 --- a/models/issues/stopwatch.go +++ b/models/issues/stopwatch.go @@ -47,7 +47,6 @@ func (err ErrIssueStopwatchAlreadyExist) Unwrap() error { type Stopwatch struct { ID int64 `xorm:"pk autoincr"` IssueID int64 `xorm:"INDEX"` - Issue *Issue `xorm:"-"` UserID int64 `xorm:"INDEX"` CreatedUnix timeutil.TimeStamp `xorm:"created"` } @@ -134,7 +133,7 @@ func StopwatchExists(userID, issueID int64) bool { } // HasUserStopwatch returns true if the user has a stopwatch -func HasUserStopwatch(ctx context.Context, userID int64) (exists bool, sw *Stopwatch, err error) { +func HasUserStopwatch(ctx context.Context, userID int64) (exists bool, sw *Stopwatch, issue *Issue, err error) { type stopwatchIssueRepo struct { Stopwatch `xorm:"extends"` Issue `xorm:"extends"` @@ -150,10 +149,10 @@ func HasUserStopwatch(ctx context.Context, userID int64) (exists bool, sw *Stopw Get(swIR) if exists { sw = &swIR.Stopwatch - sw.Issue = &swIR.Issue - sw.Issue.Repo = &swIR.Repository + issue = &swIR.Issue + issue.Repo = &swIR.Repository } - return exists, sw, err + return exists, sw, issue, err } // FinishIssueStopwatchIfPossible if stopwatch exist then finish it otherwise ignore @@ -233,12 +232,12 @@ func CreateIssueStopwatch(ctx context.Context, user *user_model.User, issue *Iss } // if another stopwatch is running: stop it - exists, sw, err := HasUserStopwatch(ctx, user.ID) + exists, sw, issue, err := HasUserStopwatch(ctx, user.ID) if err != nil { return err } if exists { - if err := FinishIssueStopwatch(ctx, user, sw.Issue); err != nil { + if err := FinishIssueStopwatch(ctx, user, issue); err != nil { return err } } diff --git a/models/issues/stopwatch_test.go b/models/issues/stopwatch_test.go index ec2778aa81f54..ea3827a1f64e2 100644 --- a/models/issues/stopwatch_test.go +++ b/models/issues/stopwatch_test.go @@ -45,12 +45,12 @@ func TestStopwatchExists(t *testing.T) { func TestHasUserStopwatch(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - exists, sw, err := issues_model.HasUserStopwatch(db.DefaultContext, 1) + exists, sw, _, err := issues_model.HasUserStopwatch(db.DefaultContext, 1) assert.NoError(t, err) assert.True(t, exists) assert.Equal(t, int64(1), sw.ID) - exists, _, err = issues_model.HasUserStopwatch(db.DefaultContext, 3) + exists, _, _, err = issues_model.HasUserStopwatch(db.DefaultContext, 3) assert.NoError(t, err) assert.False(t, exists) } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 7d9d5c7bc82c1..b3e2b652860f4 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1427,8 +1427,8 @@ func ViewIssue(ctx *context.Context) { ctx.Data["IsStopwatchRunning"] = issues_model.StopwatchExists(ctx.Doer.ID, issue.ID) if !ctx.Data["IsStopwatchRunning"].(bool) { var exists bool - var sw *issues_model.Stopwatch - if exists, sw, err = issues_model.HasUserStopwatch(ctx, ctx.Doer.ID); err != nil { + var swIssue *issues_model.Issue + if exists, _, swIssue, err = issues_model.HasUserStopwatch(ctx, ctx.Doer.ID); err != nil { ctx.ServerError("HasUserStopwatch", err) return } @@ -1436,7 +1436,7 @@ func ViewIssue(ctx *context.Context) { if exists { // Add warning if the user has already a stopwatch // Add link to the issue of the already running stopwatch - ctx.Data["OtherStopwatchURL"] = sw.Issue.Link() + ctx.Data["OtherStopwatchURL"] = swIssue.Link() } } ctx.Data["CanUseTimetracker"] = ctx.Repo.CanUseTimetracker(issue, ctx.Doer) diff --git a/routers/web/repo/issue_stopwatch.go b/routers/web/repo/issue_stopwatch.go index bbb751129cc99..3e715437e6c6c 100644 --- a/routers/web/repo/issue_stopwatch.go +++ b/routers/web/repo/issue_stopwatch.go @@ -86,7 +86,7 @@ func GetActiveStopwatch(ctx *context.Context) { return } - _, sw, err := issues_model.HasUserStopwatch(ctx, ctx.Doer.ID) + _, sw, issue, err := issues_model.HasUserStopwatch(ctx, ctx.Doer.ID) if err != nil { ctx.ServerError("HasUserStopwatch", err) return @@ -97,9 +97,9 @@ func GetActiveStopwatch(ctx *context.Context) { } ctx.Data["ActiveStopwatch"] = StopwatchTmplInfo{ - sw.Issue.Link(), - sw.Issue.Repo.FullName(), - sw.Issue.Index, + issue.Link(), + issue.Repo.FullName(), + issue.Index, sw.Seconds() + 1, // ensure time is never zero in ui } } From e1ec6d74e6e5faa867fe48349b8976bc964afda1 Mon Sep 17 00:00:00 2001 From: zeripath Date: Wed, 22 Feb 2023 15:34:00 +0000 Subject: [PATCH 3/4] Placate the linter --- models/issues/stopwatch.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issues/stopwatch.go b/models/issues/stopwatch.go index 017a8ddefc6d2..d1625115942b3 100644 --- a/models/issues/stopwatch.go +++ b/models/issues/stopwatch.go @@ -232,7 +232,7 @@ func CreateIssueStopwatch(ctx context.Context, user *user_model.User, issue *Iss } // if another stopwatch is running: stop it - exists, sw, issue, err := HasUserStopwatch(ctx, user.ID) + exists, _, issue, err := HasUserStopwatch(ctx, user.ID) if err != nil { return err } @@ -243,7 +243,7 @@ func CreateIssueStopwatch(ctx context.Context, user *user_model.User, issue *Iss } // Create stopwatch - sw = &Stopwatch{ + sw := &Stopwatch{ UserID: user.ID, IssueID: issue.ID, } From c90c60d45f6cc4c11aa63614a1dc1adf9b97ac18 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 24 Feb 2023 14:40:30 +0000 Subject: [PATCH 4/4] fix bug Signed-off-by: Andrew Thornton --- models/issues/stopwatch.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issues/stopwatch.go b/models/issues/stopwatch.go index d1625115942b3..c8cd5ad33f26a 100644 --- a/models/issues/stopwatch.go +++ b/models/issues/stopwatch.go @@ -232,12 +232,12 @@ func CreateIssueStopwatch(ctx context.Context, user *user_model.User, issue *Iss } // if another stopwatch is running: stop it - exists, _, issue, err := HasUserStopwatch(ctx, user.ID) + exists, _, otherIssue, err := HasUserStopwatch(ctx, user.ID) if err != nil { return err } if exists { - if err := FinishIssueStopwatch(ctx, user, issue); err != nil { + if err := FinishIssueStopwatch(ctx, user, otherIssue); err != nil { return err } }