search

silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 66 forks source link

AssetControlExtension should run less queries #560

Closed lekoala closed 1 year ago

lekoala commented 1 year ago

Fixes https://github.com/silverstripe/silverstripe-assets/issues/559

GuySartorelli commented 1 year ago

Have you done any profiling or other quantitative analysis that shows this as being a bottleneck, or which shows the PR as providing actual performance benefits? This feels like the sort of thing that could lead to regressions, so I'd like to see that it's actually providing a clear benefit before taking it any further.

lekoala commented 1 year ago

@GuySartorelli i just saw it leads to unecessary queries

for example: currently, the session-manager middleware update the LastAccessed property on each request (including ajax requests, so that's a ton of updates queries each time) this extension applies to all dataobjects, even when they don't have assets, so it performs a select on each write, for all dataobjects. so, currently, we have one select and one update, for each request. i don't see how an extra select on each write cannot slow things down.

there is a similar issue with the SitetreeTracking and FileTracking extensions. they are applied to all dataobjects, even when there are no fields using shortcode in it.

any dataobject without files or htmltext fields are currently doing extra selects before writing for this reason, including base models like LoginSession.

i don't have a strict benchmark, but with all the recent PR i've made, pages are displaying twice as fast on average for my custom admin module. (the login session being updated on each request being one really big offender)

michalkleiner commented 1 year ago

Just a thought that crossed my mind now — it could be good to have some additional metadata around data models (or classes in general) maybe within the config system or the manifest where we could hold information like this (has files? has fields that can have shortcodes? other stuff that can only be changed via adding/removing extensions or via dev/build) and then use it for similar optimisations? Could be a heavy weight cannon for what's needed here (in this PR), but had to write it down.

lekoala commented 1 year ago

@michalkleiner love the idea! from what I can see, the pattern is this one: some extensions need to be applied globally since you don't know which db fields would be used. ideally, it would be great that based on the fields/relations, extension get applied (or not) to a model. that would allow to keep "lightweight" (all things considered :p) models without extensions that do not apply to them.

GuySartorelli commented 1 year ago

@michalkleiner I like that idea a lot, as a general concept. I'm guessing that'd belong on DataObjectSchema?

michalkleiner commented 1 year ago

@michalkleiner I like that idea a lot, as a general concept. I'm guessing that'd belong on DataObjectSchema?

Yeah, probably, depends on what data it would hold and where it would live etc. Could be config maybe, too, since what db fields and what relations are on each data model is defined via config. Could be a build generated file that would get loaded similarly to graphql schema or similar.. just brainstorming. We probably should create an RFC ticket for that and there we might end up not having enough use cases for it and then it will end up forgotten or just a bolt on dataobjectschema.

lekoala commented 1 year ago

For reference https://github.com/silverstripe/silverstripe-framework/issues/10850

Do you want to proceed with my current approach ? as an alternative, we can simply avoid the priorRecord call instead of returning early, but i don't think it's necessary.

i don't see how my approach could go wrong : a dataobject with no assets should not make asset manipulations...

lekoala commented 1 year ago

@GuySartorelli let me know if you need anything else from my end on this

GuySartorelli commented 1 year ago

Sorry, I just haven't had time to look at this recently. It's on my radar but I can't give an estimate of when I'll have time to look at it again. If @michalkleiner of another core committer checks out your response to my concern and decides that they're happy with it, I'll have no concerns with them merging it - otherwise it'll just have to wait until someone makes time to look at it I'm afraid.

lekoala commented 1 year ago

@GuySartorelli :-) glad to help. it was really killing me to see those useless queries being made.