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

Very slow. Why by default in a new app? #7

Open
Tracked by #800
frederikhors opened this issue Nov 28, 2018 · 8 comments
Open
Tracked by #800

Very slow. Why by default in a new app? #7

frederikhors opened this issue Nov 28, 2018 · 8 comments
Assignees
Labels
enhancement New feature or request s: accepted was accepted or confirmed
Milestone

Comments

@frederikhors
Copy link

frederikhors commented Nov 28, 2018

This is extremely slow and it is a pity this is a default in a new Gobuffalo app (until performances increase...).

Can I understand why this is the default?

If I comment default lines in app.go Buffalo is much faster!

// app.Use(popmw.Transaction(models.DB))

Beego and others doesn't have transaction manager like this by default.

Is this also like Rails ActiveRecord behaviour?

I'm using just this code for some perf tests using in a new project just one resolver:

var player models.Player
if err := models.DB.Last(&player); err != nil {
  return &player, fmt.Errorf("no selected player")
}
return &player, nil
@mclark4386
Copy link
Member

It's the default, IIRC, because a) rails has ActiveRecord going by default and part of the inspiration for buffalo is rails and b) most web app need a db component and pop is the one that mark built specifically for buffalo. Please feel free to use --skip-pop in the app creation or to skip the middleware for handlers/actions/routes/groups you don't need/want it for! ( https://gobuffalo.io/en/docs/middleware#skipping-middleware )

@stanislas-m
Copy link
Member

stanislas-m commented Nov 28, 2018

This is extremely slow and it is a pity this is a default in a new Gobuffalo app (until performances increase...).

You can't say something like that without providing some numbers. Very slow is subjective and means nothing. :)

@frederikhors
Copy link
Author

frederikhors commented Nov 28, 2018

You can't say something like that without providing some numbers. Very slow is subjective and means nothing. :)

You're right. I don't have time now for pushing projects, but:

buffalo new appName:

  • add new Player model
  • add new controller with the code in first post (I will paste below)
  • migrate db
  • use vegeta (https://github.com/tsenart/vegeta)
  • results:
    • disable all but popmw-transaction: mean 30ms
    • disable all in main file (including popmw-transaction): mean 9/10ms

Same code and same db (pq) with beego new appName: results w/ vegeta: mean 3ms

var player models.Player
if err := models.DB.Last(&player); err != nil {
  return &player, fmt.Errorf("no selected player")
}
return &player, nil

@u007
Copy link
Member

u007 commented Dec 3, 2018

@frederikhors ive the same issue, i dont recommend using pop transaction middleware. in many of my cases, connection did not close properly and causing db connection to go beyond my cloud server provided

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment. Otherwise, this will be closed in 7 days.

@github-actions github-actions bot added the stale label Sep 27, 2022
@sio4 sio4 added s: triage and removed stale labels Sep 27, 2022
@sio4
Copy link
Member

sio4 commented Oct 7, 2022

First, this issue is interesting. Performance is one of the important factors for developers even though not all developers are working on a performance-sensitive application (but still they are looking for that) and sometimes productivity with reliability "out of the box" could be better for (maybe) many developers.

Anyway, the interesting part is the following:

If I comment default lines in app.go Buffalo is much faster!

// app.Use(popmw.Transaction(models.DB))

In the Buffalo way, in other simple words the controller generated by buffalo g controller or buffalo g resource, the code cannot run without the above line since they use tx, ok := c.Value("tx").(*pop.Connection). If the middleware is disabled, there is no tx in the context and the controller will return an error. So the above description is odd to me.

After reading the original post and the additional reprod steps further, it seems like I found the reason. The controller uses models.DB directly without using transaction, regardless if the middleware is enabled or not, so the middleware actually does nothing for the Last() even when it is enabled, but just made an additional empty transaction (with locking). So what happened on your test was using two connections (with locking) for every single request.

var player models.Player
if err := models.DB.Last(&player); err != nil {
  return &player, fmt.Errorf("no selected player")
}
return &player, nil

The concept of pop middleware is great. It provides database transactions transparently, so developers can just focus on the business logic then the transaction commit or rollback will be done by the middleware automatically. Also, the middleware provides timing information on the log, which is also useful.

By the way, one of my recent questions regarding Pop in my mind was how can we have better visibility on connection pool usage/management so I feel like this issue is a good input for developing the question. Also, another question was how to prevent a transaction for a non-database-related request or how to reduce the duration of a transaction.

I will keep this issue in mind once I started on the work. (I will close this issue once I open separate issues for those)

@sio4 sio4 added this to the Backlog milestone Oct 7, 2022
@sio4 sio4 self-assigned this Oct 7, 2022
@sio4 sio4 modified the milestones: Backlog, v3.1.0 Oct 7, 2022
@sio4
Copy link
Member

sio4 commented Oct 7, 2022

Oh, it looks like #28 is clearly explaining the situation of the performance issue that happened when using models.DB directly while pop middleware is enabled.

@sio4
Copy link
Member

sio4 commented Dec 16, 2022

Some test results with a simple stress test:

  • A single instance of a simple app running with debugging mode by buffalo dev
  • Postgres database in a container on the same laptop
  • Stress with the following command
echo 'GET http://localhost:3000/bmt-endpoint' \
    | vegeta attack -rate 5000 -duration 5s -max-workers 150 | tee results.bin \
    | vegeta report

Test with All()

version pool popmw tx rps latency P95 success conn peak conn left Error
v6.1.1 inf yes yes 810.4 191.6 591.6 100% 154 2
v6.1.1 inf yes no 437.4 355.7 1000 87.33% 250 2 missing
v6.1.1 inf no no 952.2 161.9 590.3 100% 151 2
v6.1.1 5 yes yes 2393.6 62.9 179.2 100% 5 2
v6.1.1 5 yes no - - - - 5 5 locked
v6.1.1 5 no no 3963.6 37.6 98.4 100% 5 2

Test with Create()

version pool popmw tx rps latency P95 success conn peak conn left Error
v6.1.1 inf yes yes 684.4 226.3 666.4 100% 156 2
v6.1.1 inf yes no 357.8 437.0 1057 79.32% 247 2 missing
v6.1.1 inf no no 501.0 309.9 927.4 100% 154 2
v6.1.1 5 yes yes 1318.8 114.5 435.8 100% 5 2
v6.1.1 5 yes no - - - - 5 5 locked
v6.1.1 5 no no 1214.6 124.6 399.0 100% 5 2

Topics that need to be discussed:

  1. disabling pop middleware by default could be considerable from the perspective of performance
  2. database access via DB pool, while pop middleware is enabled, should have a warning
  3. transaction middleware should be transparent so the application code can be uniform for both cases
  4. should have a default pool size (to reduce possible connection overhead)

It can be a global scope rather than buffalo pop. However, I will track this performance/design issue here for now.

@sio4 sio4 added enhancement New feature or request s: accepted was accepted or confirmed and removed s: triage labels Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request s: accepted was accepted or confirmed
Projects
None yet
Development

No branches or pull requests

5 participants