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

perf: improve performance of bulk child entity updates and avoid hitting the AQL limit of 500 nesting limit doing it #302

Merged
merged 3 commits into from
Nov 9, 2023

Conversation

Yogu
Copy link
Member

@Yogu Yogu commented Nov 7, 2023

perf: improve performance of bulk child entity updates and avoid hitting the AQL limit of 500 nesting limit doing it

Child entity updates generate a lot of AQL because each entity can have different kinds of updates, and some of them refer to old values. This is still the case after the optimizations.

However, previously, we performed multiple child entity updates by nesting conditional expressions like this:

items = items.map(item =>
  item.id == 2 ? update2(item) : (item.id == 1 ? update1(item) : item)
);

This generated one level of nesting per update. ArangoDB 3.11 now limits the AQL nesting to 500, which means we would be limited to a little under 500 item updates in one mutation. In addition, a few hundred updates had realy bad performance.

Now, we convert the list into a dictionary (id -> entity), so we can more efficiently look up items and construct the new list.

This optimization is only applied after a threshold of 3 (configurable) because it involves some steps, and doing a single or two updates is probably still faster with the conditionals.

This can be useful to e.g. generate predictable IDs for testing
purposes.
…ing the AQL limit of 500 nesting limit doing it

Child entity updates generate a lot of AQL because each entity can have
different kinds of updates, and some of them refer to old values. This
is still the case after the optimizations.

However, previously, we performed multiple child entity updates by
nesting conditional expressions like this:

items = items.map(item =>
  item.id == 2 ? update2(item) : (item.id == 1 ? update1(item) : item)
);

This generated one level of nesting per update. ArangoDB 3.11 now limits
the AQL nesting to 500, which means we would be limited to a little
under 500 item updates in one mutation. In addition, a few hundred
updates had really bad performance.

Now, we convert the list into a dictionary (id -> entity), so we can
more efficiently look up items and construct the new list.

This optimization is only applied after a threshold of 3 (configurable)
because it involves some steps, and doing a single or two updates is
probably still faster with the conditionals.
@Yogu Yogu requested a review from mfusser November 7, 2023 19:14
@@ -1,3 +1,4 @@
{
"authRoles": ["allusers"]
"authRoles": ["allusers"],
"childEntityUpdatesViaDictStrategyThreshold": 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to me this reads as "the threshold that needs to be reached to activate dict-strategy is 2. So it is active for 2" which is not the case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind, it is the case. I just got confused because the default is supposed to be 3 and this is a file called "default context"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I initially had the (real) default at 5 and wanted to reduce it for the tests so we have more chances to trigger it, but I then dialed down the real default to 3. No idea what a good value could be yet.

@Yogu Yogu merged commit 3713622 into main Nov 9, 2023
28 checks passed
@Yogu Yogu deleted the child-entity-updates-without-nesting branch November 9, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants