gobuffalo / buffalo-pop

A plugin to use gobuffalo/pop with buffalo
MIT License
19 stars 10 forks source link

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

Open frederikhors opened 5 years ago

frederikhors commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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:

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 commented 5 years ago

@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[bot] commented 2 years ago

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.

sio4 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

Some test results with a simple stress test:

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.