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

Avoid deep copy in entity lookup #2178

Merged
merged 3 commits into from
Jun 21, 2023
Merged

Avoid deep copy in entity lookup #2178

merged 3 commits into from
Jun 21, 2023

Conversation

krdln
Copy link
Contributor

@krdln krdln commented Jun 21, 2023

We never mutate entities directly in the maps, so copying was not necessary.
Also, added added a type wrapper so it's less likely someone tries mutating the
returned entity.

Drive-by:

  • Stop using proto-generated structs for Entities datastructure.
  • Fix lock in Clear() (fortunately, used only in tests).

Ref: #2170

Checklist
  • Tested in playground or other setup
  • Screenshot (Grafana) from playground added to PR for 15+ minute run (forgot to paste 🤦)
  • Tests and/or benchmarks are included – already covered by existing tests

Summary by OpenAI

Refactor:

  • Replaced proto-generated structs with custom Entities data structure
  • Added type wrapper to prevent direct mutation of entities
  • Removed unnecessary deep copy in entity lookup
  • Fixed lock issue in Clear() function

🎉 No more deep copy, we're swift and light,
Type wrappers guard our entities right.
Locks are fixed, and code's refined,
A better version, now combined. 🚀

krdln added 2 commits June 21, 2023 10:59
We're never mutate entities directly in the map, so that's ok.  Also,
added a type wrapper so it's less likely someone tries mutating the
returned entity.
@krdln krdln requested review from a team as code owners June 21, 2023 10:01
@krdln krdln mentioned this pull request Jun 21, 2023
16 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Jun 21, 2023

🤖 OpenAI

The changes in this pull request focus on improving performance, maintainability, and modularity by removing unnecessary deep copy in entity lookup, adding a type wrapper to prevent direct mutation of entities, replacing proto-generated structs for Entities data structure, and fixing a lock in the Clear() function.

Files Summary
pkg/discovery/entities/entities-service.go,
pkg/discovery/entities/entities.go,
pkg/discovery/entities/entities_test.go,
extensions/fluxninja/heartbeats/services_test.go
The changes involve adding a new type wrapper, removing deep copy, and fixing a lock, which can impact the behavior of the code.
pkg/policies/flowcontrol/service-getter/service-getter.go,
pkg/policies/flowcontrol/service-getter/service-getter_test.go,
pkg/policies/flowcontrol/service/check/check_test.go,
pkg/policies/flowcontrol/service/checkhttp/checkhttp_test.go,
pkg/policies/flowcontrol/service/envoy/authz_test.go
The changes are minor and do not impact the logic or functionality significantly.
pkg/discovery/entities/entities.go The changes improve performance, maintainability, and modularity by removing the deep copy of entities in maps, adding a type wrapper to avoid direct mutation, replacing proto-generated structs, and fixing a lock.

Chat with 🤖 OpenAI Bot (@openai)

  • Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
  • Invite the bot into a review comment chain by tagging @openai in a reply.

Code suggestions

  • The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
  • You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.

Ignoring further reviews

  • Type @openai: ignore anywhere in the PR description to ignore further reviews from the bot.


In the recent run, only the files that changed from the base of the PR and between 2ff7dfa89761398ba1c7aad6d25c20e8b7db4e62 and 8ee3b5ffa1c2941f53aec017974501a5b0be34a3 commits were reviewed.

Files not reviewed due to simple changes (7)

Skipped review in the recent run

  • extensions/fluxninja/heartbeats/services_test.go
  • pkg/discovery/entities/entities.go
  • pkg/discovery/entities/entities_test.go
  • pkg/policies/flowcontrol/service-getter/service-getter_test.go
  • pkg/policies/flowcontrol/service/check/check_test.go
  • pkg/policies/flowcontrol/service/checkhttp/checkhttp_test.go
  • pkg/policies/flowcontrol/service/envoy/authz_test.go

@krdln krdln merged commit b20387e into main Jun 21, 2023
@krdln krdln deleted the no-copy-entity branch June 21, 2023 10:43
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.

1 participant