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

Replace arrays with slices #680

Merged
merged 2 commits into from
Jan 22, 2019
Merged

Replace arrays with slices #680

merged 2 commits into from
Jan 22, 2019

Conversation

Allda
Copy link
Collaborator

@Allda Allda commented Jan 2, 2019

This solution should be easier to read and maintain because there is no need of using indexes.

NOTE: There are few more places which use indexes. If this PR will be approved I can refactor rest of codebase.

@jzelinskie
Copy link
Contributor

I suspect that based on the comment above the code:

// TODO(Sida): A better interface for bulk insertion is needed.

This is done for performance reasons. append is going to allocate whenever adding the new value will exceed the existing cap, then it'll double the size of the slice.

The existing code allocates only once.

Copy link
Contributor

@jzelinskie jzelinskie left a comment

Choose a reason for hiding this comment

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

I think we can get the best of both worlds by keeping the usage of make, followed by using append.

keys := make([]interface{}, len(features)*3)
for _, f := range features {
  keys = append(keys, f.Name, f.Version, f.VersionFormat)
  ...
}

database/pgsql/layer.go Outdated Show resolved Hide resolved
database/pgsql/feature.go Outdated Show resolved Hide resolved
@jzelinskie jzelinskie added component/database area/performance related to improving application performance reviewed/needs rework will be closed if review not addressed labels Jan 14, 2019
@Allda
Copy link
Collaborator Author

Allda commented Jan 15, 2019

This doesn't seem to work because make([]interface{}, len(features)*3) creates a slice with default values and append() adds items at the end.

@jzelinskie
Copy link
Contributor

Ah, you probably have to do make([]interface{}, 0, len(features)*3) so that you set the capacity and not the length.

The current code is much cleaner because it doesn't use indexes + it
should be more memory efficient.
The current code is much cleaner because it doesn't use indexes + it
should be more memory efficient.
@Allda
Copy link
Collaborator Author

Allda commented Jan 17, 2019

Now it works fine after setting just capacity.

@jzelinskie jzelinskie merged commit cc8d115 into quay:master Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance related to improving application performance reviewed/needs rework will be closed if review not addressed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants