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

[pdata] Revisit Map.Insert/Update/Upsert methods #5974

Closed
dmitryax opened this issue Aug 26, 2022 · 2 comments
Closed

[pdata] Revisit Map.Insert/Update/Upsert methods #5974

dmitryax opened this issue Aug 26, 2022 · 2 comments
Assignees
Labels
area:pdata pdata module related issues

Comments

@dmitryax
Copy link
Member

pcommon.Map.Insert/Update/Upsert are the only methods allowing setting the mutable types on the Map. This practice is generaly discouraged in pdata module since it potentially can lead to setting the same maps/slices across different parts of pdata.

Setting mutable types should be replaced with other methods, e.g. Map.UpsertEmpty() Value.

@dmitryax dmitryax added the area:pdata pdata module related issues label Aug 26, 2022
@dmitryax dmitryax self-assigned this Aug 26, 2022
@dmitryax
Copy link
Member Author

dmitryax commented Aug 27, 2022

@bogdandrutu I realized all the Upsert/Insert/Update methods copy the underlying data under the hood. It means that my concern about sharing the same objects in different parts of pdata is invalid. But it means that the only way to put a map or a slice as a child of another map is to generate a map and copy it, which always implies another extra allocation for the whole map.

childMap := NewValueMap()
... fill childMap ...
parentMap.Upsert("key", childMap) // childMap is copied here

If we move to UpsertEmptyMap, we don't have any extra allocations

childMap := parentMap.UpsertEmptyMap("key")
... fill childMap ...

So even if Upsert/Insert/Update do defensive copying, I think we should still deprecate them.

The previous behavior can be replicated with:

childMap.CopyTo(parentMap.UpsertEmptyMap("key"))

@bogdandrutu
Copy link
Member

This is done, since we removed or deprecate (last one is Insert) all the funcs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:pdata pdata module related issues
Projects
None yet
Development

No branches or pull requests

2 participants