Skip to content

importer: reuse pluralize client to improve performance#5338

Open
wuxu92 wants to merge 5 commits intohashicorp:mainfrom
wuxu92:import-fix-pluralize-client
Open

importer: reuse pluralize client to improve performance#5338
wuxu92 wants to merge 5 commits intohashicorp:mainfrom
wuxu92:import-fix-pluralize-client

Conversation

@wuxu92
Copy link
Copy Markdown
Collaborator

@wuxu92 wuxu92 commented Apr 20, 2026

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

The current pluralize function initialize the client for every call, it caused huge unnecessary memory allocations and caused GC increased a lot. by simply reusing the client, we can reduce the allocation a lot and import faster.

below is a experiment on importing Compute@2025-04-01, the allocations of pluralize client decreased from 4121064 (33.7%) to 9841 (0.1%) by this PR. and import time cost from 143s -> 117s.

Not reuse pluralize client:

image

Reuse pluralize client:
image

PR Checklist

  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so, please include appropriate closing keywords below.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.

Copy link
Copy Markdown
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks @wuxu92 - I think we can get even more out of this by removing the mutex which I don't believe is necessary? (unless you have testing evidence to show it is?)

pluralizeClient *pluralize.Client
once sync.Once
// pluralizeClient is not thread safe
mux sync.Mutex
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this is actually thread safe, do you have evidence to the contrary? (Singular() and Plural() are "read-only" and don't have any internal state change to lock for, so should be fine?)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Updated, Thanks! it's safe as we only call Singular and Plural here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants