silverstripe / silverstripe-session-manager

Allow users to manage and revoke access to multiple login sessions across devices.
BSD 3-Clause "New" or "Revised" License
9 stars 6 forks source link

Avoid updating the loginSession on each request #149

Closed lekoala closed 1 year ago

lekoala commented 1 year ago

I remember somewhere that part of the idea of removing the LastVisited field from the member was to avoid running on UPDATE query on each request.

This bit

https://github.com/silverstripe/silverstripe-session-manager/blob/c5ca5eb7458f10c941d0aa52321e760955dfbc18/src/Middleware/LoginSessionMiddleware.php#L45-L48

is actually running on each request, and update request are non trivial

i don't think that's really necessary to have that level of precision. Maybe we could run the update if the diff between the last time and the current time is above a certain threshold (configurable) it would be a matter of adding an option, which can be set to 0 if you really want to update on each request, and otherwise have the update done if the diff is greater than 5 minutes

any opinions ?

PRs

maxime-rainville commented 1 year ago

There's a garbage collection process attach to this. https://github.com/silverstripe/silverstripe-session-manager/blob/c5ca5eb7458f10c941d0aa52321e760955dfbc18/src/Services/GarbageCollectionService.php#L26-L34

Session that are non-persistent and are longer than the default lifetime of 3600 are at risk of being garbage collected. https://github.com/silverstripe/silverstripe-session-manager/blob/c5ca5eb7458f10c941d0aa52321e760955dfbc18/src/Models/LoginSession.php#L97

If we decide to address this, we should consider the impact on this side process.

lekoala commented 1 year ago

@maxime-rainville yes, i think if we add the threshold to the lifetime that would work nicely

removing the update query from each query seems like a real performance enhancement, considered that many cms pages do ajax requests (for the gridfield, search form, etc). from what i can see, that query is basically one of the most expensive on a regular page load.

lekoala commented 1 year ago

i've updated the PR while at it, i've also used DBDatetime::now() in the garbage collection service to allow mocking time (not sure how this is unit tested at the moment)

lekoala commented 1 year ago

@GuySartorelli so here is to give you an idea of how slow this is

here is a trimmed down page only running a few queries with a logged in user

10 statements were executed => 27.78ms vs 8 statements were executed => 2.04ms

the two extra statements

SELECT count(*) AS "Count" FROM "LoginSession" WHERE ("ID" = 248740) => 223μs

UPDATE "LoginSession" SET "LastAccessed" = '2023-06-28 10:38:03', "LastEdited" = '2023-06-28 10:38:03' WHERE ("ID" = 248740) => 22.02ms

so yes, it's possible to shove 20ms on my dev machine. obviously, this change is less obvious on many pages in the cms where a lot of queries are being made

i'm not running debugbar in prod so i don't have actual prod benchmark to show this, but that would be logical that a write on the database is slower than a pure read request.