Performance improvements: Add Brainstorming Label to idea + broadcast lane update #507
Conversation
PragTob
force-pushed
the
performance-improvements
branch
from
bc36a24 to
966eed0
Compare
|
I think we can easily remove 2 more database queries: def handle_event(
"add_idea_label_to_idea",
%{
"idea-id" => idea_id,
"idea-label-id" => idea_label_id
},
socket
) do
idea = Ideas.get_idea!(idea_id)
idea_label = IdeaLabels.get_idea_label(idea_label_id)
IdeaLabels.add_idea_label_to_idea(idea, idea_label)
{:noreply, socket}
endWe don't need to load the idea or idea_label - we only need the ids. However, later |
|
Thanks, thats a good start of cleaning up! All the broadcasts can be improved a lot, we often load e.g. an idea or lane but in the end we just need an id. I think that happens actually in more places.
This is an interesting one, where I am open for feedback. The reason why we often broadcast a whole lane or all lanes is, that if you drag an idea, you change the complete ordering of all ideas. So it's not enough to just update idea currently. This could be done just for content changes. But if the position is changed, we need to update at least the lane where the idea is on and maybe even more lanes, if the idea was moved from lane A to B. Thats what makes it complicated and this is a very easy but improvable solution. One solution here would be to do the ordering on the socket / client side and not to rely on ordering them before the broadcast happens. Then I guess (?) just updating one idea would be sufficient. Then the question is, if you have 10 clients, if it's more efficient to order 10x instead of a single db query. But I think you gave me already the answer to this :D Feely free to have a look at this. |
|
|
|
Also interesting question in general: Passing ids vs passing objects (refs). Passing the id, you always need to refetch the object in the designated method. On the other hand, always passing the full object might lead to strange errors as for an update method you need the most recent object usually (just happened to me). |
1. IdeaIdeaLabel: Only create a new join table entry vs. processing lots We don't end up using the idea in the code that calls `IdeaLabels.add_idea_label_to_ideas` - we broadcast and load all the brainstormings and lanes again. So, there's no need to adjust the `Idea` struct to be fine. This saves us some queries (I think) but at the very least some code and some CPU cycles. 2.Do not trigger preloads for brainstorming when unneeded
PragTob
force-pushed
the
performance-improvements
branch
from
966eed0 to
ce34983
Compare
|
Yeah it's all a big game of tradeoffs.
In short, complexy and increased chances for bugs vs. lower load on the system and quicker response times. For the kind of project this is, I'd assume that going with the simpler "safer" implementation is fine. That said, it feels a bit "wasteful" in my mind.
Some functions are also built to handle both through a pattern match in the function head to deal with both being passed an id or being passed the full record. |
* idea is already part of the state of the component (and we only need id and brainstorming_id) * idea_label we only need the id anyhow
|
@JannikStreek should be ready! |
This was mostly me looking at SQL emitted when adding a new label.
This PR reduces the total amount of SQL queries issued for adding a label from 20 to 14
Log for add label before optimization
Log for add label after optimization
The PR mainly does 2 things:
ideastruct afterwards anyhowThere is some further potential here, I'll highlight some and will check out some more one of these days for similar patterns.