Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: add mcache #46

Merged
merged 6 commits into from
Oct 21, 2024
Merged

perf: add mcache #46

merged 6 commits into from
Oct 21, 2024

Conversation

vm-001
Copy link
Member

@vm-001 vm-001 commented Oct 10, 2024

Summary

Add MCache(multiple levels cache) to improve performance.

MCache Implementations

The MCache uses LRU as the L1 cache and redis as the L2 cache. To invalidate the L1 cache of different WebhookX nodes, it uses Notify / Listen to broadcast events.

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 79.64912% with 58 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
db/dao/dao.go 68.51% 12 Missing and 5 partials ⚠️
eventbus/eventbus.go 71.92% 14 Missing and 2 partials ⚠️
app/app.go 60.60% 11 Missing and 2 partials ⚠️
mcache/mcache.go 80.32% 8 Missing and 4 partials ⚠️
Flag Coverage Δ
integration 66.67% <78.24%> (-0.89%) ⬇️
unit 14.30% <15.08%> (+0.55%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
config/config.go 9.43% <ø> (-2.77%) ⬇️
config/database.go 76.92% <100.00%> (-7.70%) ⬇️
constants/constants.go 100.00% <100.00%> (ø)
db/dao/attempt_dao.go 100.00% <100.00%> (ø)
db/dao/attempt_detail_dao.go 89.65% <100.00%> (+0.76%) ⬆️
db/dao/endpoint_dao.go 100.00% <100.00%> (ø)
db/dao/event_dao.go 71.87% <100.00%> (+1.87%) ⬆️
db/dao/plugin_dao.go 100.00% <100.00%> (ø)
db/dao/source_dao.go 100.00% <100.00%> (ø)
db/dao/workspace_dao.go 100.00% <100.00%> (ø)
... and 9 more

... and 48 files with indirect coverage changes

@vm-001 vm-001 force-pushed the perf/mcache branch 2 times, most recently from 98fcbc1 to 10fd2c5 Compare October 12, 2024 03:07
@vm-001 vm-001 marked this pull request as ready for review October 12, 2024 14:04
@vm-001 vm-001 force-pushed the perf/mcache branch 3 times, most recently from 3889bab to fdd484d Compare October 13, 2024 02:24
Base automatically changed from perf/batch to main October 13, 2024 07:56
@vm-001 vm-001 force-pushed the perf/mcache branch 4 times, most recently from bbfc603 to 6b0a054 Compare October 14, 2024 15:41
@vm-001 vm-001 requested a review from cchenggit October 14, 2024 15:41
@vm-001 vm-001 force-pushed the perf/mcache branch 3 times, most recently from 7f89329 to fe38e83 Compare October 14, 2024 16:02
db/dao/dao.go Outdated
if e := mcache.Invalidate(ctx, key); e != nil {
dao.log.Warnf("failed to invalidate mcache: key=%s, %v", key, err)
}
dao.publishEvent(eventbus.EventInvalidation, map[string]interface{}{
Copy link
Collaborator

Choose a reason for hiding this comment

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

use go routine to publish if not care returning

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM.

app/app.go Outdated
app.bus = eventbus.NewDatabaseEventBus(cfg.DatabaseConfig.GetDSN(), app.log)
app.bus.Subscribe(eventbus.EventInvalidation, func(data []byte) {
maps := make(map[string]interface{})
if err := json.Unmarshal(data, &maps); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

declare subscribe handlers somewhere else, keep app init code clean

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM. Updated.

)
return sql.Open("postgres", dsn)
func (cfg DatabaseConfig) GetDSN() string {
dsn := cfg.DSN
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
dsn := cfg.DSN
dsn := cfg.dsn

cfg.Port,
cfg.Database,
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
cfg.dsn = dsn
}

"fmt"
)

type DatabaseConfig struct {
DSN string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
DSN string
dsn string

already have GetDSN() method, should keep DSN private.

Copy link
Member Author

@vm-001 vm-001 Oct 16, 2024

Choose a reason for hiding this comment

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

I was going to add a new configuration field for the database section. For example:

database:
  dsn: postgres://username:[email protected]:5432/webhookx?sslmode=disable
  ...

The dsn will have the highest priority compared the the rest of all (username, password, etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

deleted. (will be done on another PR)

handler(payload.Data)
}
}
case <-time.After(90 * time.Second):
Copy link
Collaborator

Choose a reason for hiding this comment

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

DO NOT USE time.After in for select, it will cause memory leak, problem still exist in go 1.22

Copy link
Member Author

Choose a reason for hiding this comment

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

I think timer.After does not cause permanent leak, because GC will capture it when the underlying timer expires.

I replaced it by time.NewTimer() for the efficiency perspective.


var defaultOpts LoadOptions

func Load[T any](ctx context.Context, key string, opts *LoadOptions, cb Callback[T], id string) (*T, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As opts could be nil, use ... Option chain to override defaultOpts for better experience.
for example:

func Load[T any](ctx context.Context, key string, cb Callback[T], id string, opts ...Option) (*T, error) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding ... Option at the end of the function arguments make things a bit complex. I prefer to use an Options struct here.

db/dao/dao.go Outdated Show resolved Hide resolved
Co-authored-by: webhookx-x <[email protected]>
Signed-off-by: Yusheng Li <[email protected]>
@webhookx-x
Copy link
Member

The eventbus can also be used for event router rebuild when source get changed.

@webhookx-x webhookx-x merged commit 44453a0 into main Oct 21, 2024
6 checks passed
@webhookx-x webhookx-x deleted the perf/mcache branch October 21, 2024 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants