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

InMemoryBackingStore performs poorly when reading properties #347

Closed
Le-Merle opened this issue Aug 30, 2024 · 2 comments · Fixed by #368
Closed

InMemoryBackingStore performs poorly when reading properties #347

Le-Merle opened this issue Aug 30, 2024 · 2 comments · Fixed by #368
Assignees
Labels
enhancement New feature or request WIP

Comments

@Le-Merle
Copy link
Contributor

The BackingStore tries to keep the "dirty" state up to date on every call to read a property value. Because of the way the logic to track changes on collections was implemented, this has a detrimental impact on both CPU and memory consumption.

Currently, reading a property from a model is an operation of potentially exponential complexity that allocates memory on the heap.

Whether paying this cost on every property read in order to prevent sending additional data on the network is worth it or not is debatable, but it definitely isn't if we have no intention of performing any update call.

From my understanding of the implementation, the idea is to always return every values except when ReturnOnlyChangedValues is set to true. This seems to mostly be activated right before serialization, and deactivated immediately after.

If that is the case, I would suggest checking the value of ReturnOnlyChangedValues before calling EnsureCollectionPropertyIsConsistent when reading a property value. There is no rush to keep the dirty state synchronized until we are actually using it. On that note, it might also be a good idea to call it within EnumerateKeysForValuesChangedToNull, although I haven't exactly explored every intended usage of this function.

A local test with this modification applied produced large improvements on speed and removed any memory allocation when reading properties.

Is my understanding of the intent of InMemoryBackingStore correct? Would my suggested fix make sense?

@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Aug 30, 2024
@baywet
Copy link
Member

baywet commented Aug 31, 2024

Hi @Le-Merle
Thanks for using kiota and for reaching out.

The changes you're proposing sound coherent, I might be missing some details here.

Can you please send a pull request so we can discuss the changes in details?

@baywet baywet added enhancement New feature or request status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Aug 31, 2024
@baywet baywet moved this from Needs Triage 🔍 to Todo 📃 in Kiota Aug 31, 2024

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

@baywet baywet removed Status: No Recent Activity status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Sep 4, 2024
@baywet baywet linked a pull request Sep 4, 2024 that will close this issue
@github-project-automation github-project-automation bot moved this from Todo 📃 to Done ✔️ in Kiota Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants